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

Comments

Projects
None yet
6 participants
@LaszloLango

LaszloLango commented Apr 3, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

RonEld commented Apr 3, 2018

@LaszloLango I believe #814 fixes this issue

@hanno-arm

This comment has been minimized.

Contributor

hanno-arm commented Apr 3, 2018

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

@RonEld

This comment has been minimized.

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-arm

This comment has been minimized.

Contributor

hanno-arm commented Apr 3, 2018

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

@RonEld

This comment has been minimized.

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-arm

This comment has been minimized.

Contributor

hanno-arm commented Apr 3, 2018

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

@RonEld

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

ciarmcom commented Apr 3, 2018

ARM Internal Ref: IOTSSL-2206

@ciarmcom ciarmcom added the mirrored label Apr 3, 2018

@RonEld

This comment has been minimized.

Contributor

RonEld commented Apr 9, 2018

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

redtangent added a commit to redtangent/mbedtls that referenced this issue Jun 24, 2018

Add ebx to the i386 clobber list for MPI assembly
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 ARMmbed#1550.

redtangent added a commit to redtangent/mbedtls that referenced this issue Jun 24, 2018

Add ebx to the i386 clobber list for MPI assembly
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 ARMmbed#1550.
@redtangent

This comment has been minimized.

Contributor

redtangent commented Jun 24, 2018

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

Add ebx to the i386 clobber list for MPI assembly
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 ARMmbed#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

Add ebx to the i386 clobber list for MPI assembly
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 ARMmbed#1550.

redtangent added a commit to redtangent/mbedtls that referenced this issue Jul 10, 2018

@sbutcher-arm

This comment has been minimized.

Collaborator

sbutcher-arm commented Jul 20, 2018

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

@LaszloLango

This comment has been minimized.

LaszloLango commented Jul 23, 2018

Will this fix be in the next release?

@RonEld

This comment has been minimized.

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

bn_mul.h: fix x86 PIC ASM compilation with GCC<5
Fixes ARMmbed#1910

With ebx added to the MULADDC_STOP clobber list to fix ARMmbed#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

bn_mul.h: fix x86 PIC inline ASM compilation with GCC < 5
Fixes ARMmbed#1910

With ebx added to the MULADDC_STOP clobber list to fix ARMmbed#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