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

Valgrind reports "Source and destination overlap in memcpy" on bignum.c #4387

Closed
mikaleppanen opened this issue Apr 21, 2021 · 2 comments
Closed
Labels
bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d)

Comments

@mikaleppanen
Copy link


Description

  • Type: Bug
  • Priority: Major

Bug

OS
Linux

mbed TLS build:
Version: 9a1c092
OS version: Ubuntu 16.04 LTS and 20.04 LTS
Configuration: -
Compiler and options (if you used a pre-built binary, please indicate how you obtained it): gcc
Additional environment information: Valgrind memory analyzer in use

Description

On Ubuntu 16.04 LTS Wi-SUN (automated) mesh simulator environment, when running tests under Valgrind memory analyzer, we are seeing following error on memcpy on bignum.c:

"Source and destination overlap in memcpy(0x5a604a0, 0x5a604a0, 40)"

Code line:

https://github.com/ARMmbed/mbedtls/blob/218da3fcf9015103574085c44e006397800e584a/library/bignum.c#L1391

Error occurs when Wi-SUN TLS client calls mbedtls_x509_crt_parse() for it's own certificate on:

https://github.com/ARMmbed/mbed-os/blob/9738b27c7df897c29e9769911d6794ba3e5b3f19/connectivity/nanostack/sal-stack-nanostack/source/Security/protocols/tls_sec_prot/tls_sec_prot_lib.c#L222

Here's full valgrind XML report:

valgrind_log.zip

When running tests manually on Ubuntu 20.04 LTS Valgrind does not report the error. However if I add manually check for addresses before the memcpy, the code goes to assert branch:

  if( A->n > n ) {
       if (X->p + n == A->p + n) {
           printf("memcpy addresses %p %p\r\n", X->p + n, A->p + n);
           assert(0);
       }
       memcpy( X->p + n, A->p + n, ( A->n - n ) * ciL );
   }
@gilles-peskine-arm
Copy link
Contributor

This does look like a bug in mbedtls_mpi_sub_abs, triggered when X and A are aliased.

I think that in practice, it won't result in any bad behavior, because memcpy is either invoked on disjoint buffers or on exactly the same pointers. It can't be invoked on partially overlapping buffers. While the behavior is undefined, I expect that in practice, memcpy(p, p, n) is a no-op.

We do have a unit test for this, which we run with Valgrind and which passed on our CI which runs Ubuntu 16.04. I wonder why Valgrind is sometimes picking it up and sometimes not.

@tom-cosgrove-arm
Copy link
Contributor

Duplicate of #6701, and fixed by #6942

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 size-s Estimated task size: small (~2d)
Projects
None yet
Development

No branches or pull requests

6 participants