Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddressSanitizer: SEGV mbedtls/library/bignum.c:1165 mpi_mul_hlp #1550

Closed
LaszloLango opened this issue Apr 3, 2018 · 15 comments · Fixed by #1986
Closed

AddressSanitizer: SEGV mbedtls/library/bignum.c:1165 mpi_mul_hlp #1550

LaszloLango opened this issue Apr 3, 2018 · 15 comments · Fixed by #1986
Labels
bug component-crypto Crypto primitives and low-level interfaces

Comments

@LaszloLango
Copy link

Description

  • Type: Bug
  • Priority: Major

Bug

OS
linux

mbed TLS build:
Version: 2.8.0 (8be0e6d)
OS version: Ubuntu 16.04.9
Configuration: default configuration
Compiler:gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
Options: CFLAGS="-fsanitize=address -m32 -g"

Expected behavior
Returns 0

Actual behavior

ASAN:SIGSEGV
=================================================================
==28637==ERROR: AddressSanitizer: SEGV on unknown address 0x03f8371a (pc 0x080633a9 bp 0xffa40068 sp 0xffa3ff90 T0)
    #0 0x80633a8 in mpi_mul_hlp mbedtls/library/bignum.c:1165
    #1 0x8063a96 in mbedtls_mpi_mul_mpi mbedtls/library/bignum.c:1205
    #2 0x8063d1a in mbedtls_mpi_mul_int mbedtls/library/bignum.c:1229
    #3 0x80648f2 in mbedtls_mpi_div_mpi mbedtls/library/bignum.c:1397
    #4 0x8065298 in mbedtls_mpi_mod_mpi mbedtls/library/bignum.c:1469
    #5 0x809150e in mbedtls_rsa_deduce_crt mbedtls/library/rsa_internal.c:465
    #6 0x80509f0 in mbedtls_rsa_complete mbedtls/library/rsa.c:319
    #7 0x804e251 in pk_parse_key_pkcs1_der mbedtls/library/pkparse.c:758
    #8 0x804f525 in mbedtls_pk_parse_key mbedtls/library/pkparse.c:1172
    #9 0x8048e3b in main mbedtls-asan-error.c:36
    #10 0xf6fbd636 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18636)
    #11 0x8048f0f  (mbedtls-asan-error+0x8048f0f)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV mbedtls/library/bignum.c:1165 mpi_mul_hlp

Steps to reproduce

  1. compile mbedtls libraries:
    cd $MBEDTLS_HOME
    make clean
    CFLAGS="-fsanitize=address -m32 -g" make lib
  2. compile sample (see the code block below):
    cd $HOME
    gcc mbedtls-asan-error.c -o mbedtls-asan-error -O2 -fsanitize=address -m32 -g -I$MBEDTLS_HOME/include -L$MBEDTLS_HOME/library -lmbedcrypto

Code:

#include "mbedtls/ssl.h"

const char SSL_PKEY[] =
"-----BEGIN RSA PRIVATE KEY-----\n"
"MIIEpAIBAAKCAQEAyPlnutdMskKWVd+Rf2Z8Bgp+oJgciT16izkuWglVZk2ttQ1Q\n"
"vvhj1wqRkqlrOWv5ZbsEOkx3SbcMEoZ2SSKs5DINhc8UlNB8H4TBrX+jBbZgW6U+\n"
"sgPkCW5jWucveN32AcptPSgK2fD3UAloQQu9tsAkE3u+1wLlpr/f1SrjPOZnR5EO\n"
"A9NwRWKY9x+ivTfrmKozuUI/Tv8X2vtiIJMoIj9ZVzxpGolclQmW61qVAuTx5xi8\n"
"RbRO7zgUe32+gM4KmKwIvnX4mOyo1438XqWlGkSM0npEbWvmqtthldUeWUgocRne\n"
"qbIk2n2Yg/O8ghLEFY+vSvB8XdYrEvZKY/ZSIQIDAQABAoIBAAO6nPIajJHCKoft\n"
"XgW/IQ37o93W9LCXM27h4LpK8gqz5kU1ugnREgkraQgPnK20EyyQC8QJJy12+AJf\n"
"/FHfEtYpF5ckAH3CYIs1K2LMu3mfqKoKpt8ms1R5d95366mSdL4Tw3MfCxSAJBaY\n"
"Fhce4KZDamfyI9hZdWlipgSOhyjaFSH6SdiS8u8P9SV9ySHNfRX4UPQ7a5HUg2FG\n"
"sXCObFvSdAkkpAzNl+1gzAO9cy6hOjS4epHa3P/B0uGG6/FHJbT1zFKBJxGi2WFn\n"
"lf0jURU1q8026gaoh3sgrK3yX00Q9M/hIkVUaUYXzdMv2sPYP+fxECtAOkSUDipb\n"
"qZsdAAECgYEA8Hsy+7uFNpyo6Q07sZ3fbsnk+uAI9Blw6Ezz4nsGEAHQ7bXqQUOh\n"
"wyWItyL57KwwVSaTlVCZwp2V8tU4ybiyhf1FyWJgSwFoj2XJMOlR2EmqNUjXN91X\n"
"mV62hWJIJhtRaQKEGT9JZsGDLfPuen9gFUjXmFbe37Tf5A/NpUVZGGECgYEA1fGK\n"
"vXgjYagd2o5TY7xEgtEAZV1bBQ0d7lYDjvZsDzug+gfqU1MwtXEJAIMEQL4ywI1t\n"
"uP4QryK5J65PmBuq474HqWa4txwqM38J+Z8oYplJSXTaybSYD+Vf2R+KLkFHMchH\n"
"2EPzJE6lYYMJDrSW8iC5wCaC+fbJ6QfTmZZ8kcECgYEAoUBrImNOYx1PId6OvX33\n"
"+YkFsreRKWT50brv+li16vvcxdiquJKKIJnFf8/DOFEJo79XTNMcF2SlzIvvJUxk\n"
"4PXA2tXNbd4G58i/zL1W9SoIKOyr67jO6XeZ+fy6FltRDpHyVB+cr3to4+Jicd+B\n"
"ZSRP9MWjcuwNCRcTtRO4N2ECgYAEM40c8WoIdeu4KglbMQxLYV1XoEC0VbCbyJaj\n"
"TRWMKwibQGKKplyTg5fAqdIAj3uhqmVYN60OM2ldbR/lBc4SUN4HppvEBMqTXlBM\n"
"1aJOZWI6DhBp26EM1t1N/z+Qbvm98YfvqE3zDZRT2OXpowQ/1wKu0lLKI92NNPkj\n"
"z/+8QQKBgQDPCwtgT2m8SGcRM0MtYK2FSXDrQIXDISQ/c2/7Xrhzx4C7anj2z0Bp\n"
"BjT9ufv86DkZUr31ObP6B8SW9J9EPlQRhHOD6h2XIPs+stxEbm1LvQGVU1sFA/C9\n"
"Un0H1iDxSOnLjK1StjWwYfIy6SHFRLYOAmX0BPglJ+5p6Eq/dc96PQ==\n"
"-----END RSA PRIVATE KEY-----\n"
;

int main(void) {
  mbedtls_pk_context pkey;
  mbedtls_pk_init(&pkey);
  mbedtls_pk_parse_key(&pkey, (const unsigned char *) SSL_PKEY, sizeof(SSL_PKEY), NULL, 0);
  mbedtls_pk_free(&pkey);
  return 0;
}

Additional information:

The error happens with every generated key (both openssl and mbedtls), e.g:
openssl genrsa -out ryans-key.pem 2048
or
programs/pkey/gen_key type=rsa rsa_keysize=4096 filename=our_key.key

@RonEld
Copy link
Contributor

RonEld commented Apr 3, 2018

@LaszloLango Thank you for raising this issue!
We will look into this.
Note: This happens only with the -m32 flag defined

@RonEld
Copy link
Contributor

RonEld commented Apr 3, 2018

@LaszloLango I believe #814 fixes this issue

@hanno-becker
Copy link

@RonEld It just disables the relevant assembly, though, not identifying the underlying problem.

@RonEld
Copy link
Contributor

RonEld commented Apr 3, 2018

@hanno-arm yes, it's a workaround.
But investigating the full assembly code \ implementing assembly code for 32 bit will probably take long time, which I don't see the added value in it

@hanno-becker
Copy link

@RonEld There is definitely added value -- it's not acceptable to have assembly which is broken for unknown reasons in our library.

@RonEld
Copy link
Contributor

RonEld commented Apr 3, 2018

@hanno-arm let me rephrase.
This assembly code is still used for more than i386.
Now, this assembly will not be used for i386 and it will use the c code.
I meant I don't see added value at the moment on investing resources on 32 bit linux platforms

@hanno-becker
Copy link

@RonEld Is it? The workaround adds a #if 0 around the respective assembly.

@RonEld
Copy link
Contributor

RonEld commented Apr 3, 2018

@hanno-arm correct, it was supposed to be for i386 , it's a copy\ paste error from same implementation for sparc

@RonEld
Copy link
Contributor

RonEld commented Apr 3, 2018

@hanno-arm I fixed the workaround, to avoid i386 only until it will be resolved, if we decide to invest resources in it

@ciarmcom
Copy link

ciarmcom commented Apr 3, 2018

ARM Internal Ref: IOTSSL-2206

@RonEld
Copy link
Contributor

RonEld commented Apr 9, 2018

ARM Internal Ref: IOTSSL-1139 , as 2206 is a duplicate

@simonbutcher simonbutcher added the component-crypto Crypto primitives and low-level interfaces label Jun 13, 2018
redtangent added a commit to redtangent/mbedtls that referenced this issue Jun 24, 2018
This fix adds the ebx register to the clobber list for the i386 inline assembly
for the multiply helper function.

ebx was used but not listed, so when the compiler chose to also use it, ebx was
getting corrupted. I'm surprised this wasn't spotted sooner.

Fixes Github issues Mbed-TLS#1550.
redtangent added a commit to redtangent/mbedtls that referenced this issue Jun 24, 2018
This fix adds the ebx register to the clobber list for the i386 inline assembly
for the multiply helper function.

ebx was used but not listed, so when the compiler chose to also use it, ebx was
getting corrupted. I'm surprised this wasn't spotted sooner.

Fixes Github issues Mbed-TLS#1550.
@redtangent
Copy link
Contributor

I've just posted a fix as PR #1778 .

redtangent added a commit to redtangent/mbedtls that referenced this issue Jun 24, 2018
redtangent added a commit to redtangent/mbedtls that referenced this issue Jul 10, 2018
This fix adds the ebx register to the clobber list for the i386 inline assembly
for the multiply helper function.

ebx was used but not listed, so when the compiler chose to also use it, ebx was
getting corrupted. I'm surprised this wasn't spotted sooner.

Fixes Github issues Mbed-TLS#1550.
redtangent added a commit to redtangent/mbedtls that referenced this issue Jul 10, 2018
redtangent added a commit to redtangent/mbedtls that referenced this issue Jul 10, 2018
This fix adds the ebx register to the clobber list for the i386 inline assembly
for the multiply helper function.

ebx was used but not listed, so when the compiler chose to also use it, ebx was
getting corrupted. I'm surprised this wasn't spotted sooner.

Fixes Github issues Mbed-TLS#1550.
redtangent added a commit to redtangent/mbedtls that referenced this issue Jul 10, 2018
@simonbutcher
Copy link
Contributor

The PR's for this issue (#1778, #1849 and #1850) are now merged, so this issue can be closed.

@LaszloLango
Copy link
Author

Will this fix be in the next release?

@RonEld
Copy link
Contributor

RonEld commented Jul 23, 2018

@LaszloLango the fix has been merged and backported, so it should be in the next release

jacmet added a commit to jacmet/mbedtls that referenced this issue Aug 27, 2018
Fixes Mbed-TLS#1910

With ebx added to the MULADDC_STOP clobber list to fix Mbed-TLS#1550, the inline
assembly fails to build with GCC<5 in PIC mode with the following error:

bn_mul.h:46:13: error: PIC register clobbered by ‘ebx’ in ‘asm’

This is because older GCC versions treated the x86 ebx register (which is
used for the GOT) as a fixed reserved register when building as PIC.

This is fixed by an improved register allocator in GCC 5+.  From the release
notes:

Register allocation improvements: Reuse of the PIC hard register, instead of
using a fixed register, was implemented on x86/x86-64 targets.  This
improves generated PIC code performance as more hard registers can be used.

https://www.gnu.org/software/gcc/gcc-5/changes.html

As a workaround, detect this situation and disable the inline assembly,
similar to the MULADDC_CANNOT_USE_R7 logic.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
jacmet added a commit to jacmet/mbedtls that referenced this issue Aug 27, 2018
Fixes Mbed-TLS#1910

With ebx added to the MULADDC_STOP clobber list to fix Mbed-TLS#1550, the inline
assembly fails to build with GCC < 5 in PIC mode with the following error:

include/mbedtls/bn_mul.h:46:13: error: PIC register clobbered by ‘ebx’ in ‘asm’

This is because older GCC versions treated the x86 ebx register (which is
used for the GOT) as a fixed reserved register when building as PIC.

This is fixed by an improved register allocator in GCC 5+.  From the release
notes:

Register allocation improvements: Reuse of the PIC hard register, instead of
using a fixed register, was implemented on x86/x86-64 targets.  This
improves generated PIC code performance as more hard registers can be used.

https://www.gnu.org/software/gcc/gcc-5/changes.html

As a workaround, detect this situation and disable the inline assembly,
similar to the MULADDC_CANNOT_USE_R7 logic.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
tom-cosgrove-arm pushed a commit to tom-cosgrove-arm/mbedtls that referenced this issue Jul 19, 2022
Fixes Mbed-TLS#1910

With ebx added to the MULADDC_STOP clobber list to fix Mbed-TLS#1550, the inline
assembly fails to build with GCC < 5 in PIC mode with the following error:

include/mbedtls/bn_mul.h:46:13: error: PIC register clobbered by ‘ebx’ in ‘asm’

This is because older GCC versions treated the x86 ebx register (which is
used for the GOT) as a fixed reserved register when building as PIC.

This is fixed by an improved register allocator in GCC 5+.  From the release
notes:

Register allocation improvements: Reuse of the PIC hard register, instead of
using a fixed register, was implemented on x86/x86-64 targets.  This
improves generated PIC code performance as more hard registers can be used.

https://www.gnu.org/software/gcc/gcc-5/changes.html

As a workaround, detect this situation and disable the inline assembly,
similar to the MULADDC_CANNOT_USE_R7 logic.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
daverodgman pushed a commit that referenced this issue Jul 21, 2022
Fixes #1910

With ebx added to the MULADDC_STOP clobber list to fix #1550, the inline
assembly fails to build with GCC < 5 in PIC mode with the following error:

include/mbedtls/bn_mul.h:46:13: error: PIC register clobbered by ‘ebx’ in ‘asm’

This is because older GCC versions treated the x86 ebx register (which is
used for the GOT) as a fixed reserved register when building as PIC.

This is fixed by an improved register allocator in GCC 5+.  From the release
notes:

Register allocation improvements: Reuse of the PIC hard register, instead of
using a fixed register, was implemented on x86/x86-64 targets.  This
improves generated PIC code performance as more hard registers can be used.

https://www.gnu.org/software/gcc/gcc-5/changes.html

As a workaround, detect this situation and disable the inline assembly,
similar to the MULADDC_CANNOT_USE_R7 logic.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants