mpi_add_mpi fails when all arguments are the same mpi (in-place doubling)

[rpls]

Problem
The mbedtls_mpi_add_mpi routine produces incorrect results when used as follows:

mbedtls_mpi_add_mpi( X, X, X );

I have attached a patch for the mpi test suite, demonstrating the behaviour. The behaviour doesn't occur all the time, I think only if the result mpi is grown.

V2.1.1 is affected, I have not tested other versions.

Workarround
Use mbedtls_mpi_shift_l( X, 1 ) to double an integer in-place.

Solution
I propose one of the following solutions:

1. Fix the routine (and check if other routines are affected, I only checked the addition).
2. Clarify behaviour in the documentation, if this is deliberate. At least the multiplication allows squaring an integer in place, so I guess this is a bug?

diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function
index 72b4940..c42ae0b 100644
--- a/tests/suites/test_suite_mpi.function
+++ b/tests/suites/test_suite_mpi.function
@@ -428,17 +428,35 @@ exit:
 void mbedtls_mpi_add_mpi( int radix_X, char *input_X, int radix_Y, char *input_Y,
                   int radix_A, char *input_A )
 {
-    mbedtls_mpi X, Y, Z, A;
+    mbedtls_mpi X, Y, Z, A, T1, T2, T3;
     mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &Z ); mbedtls_mpi_init( &A );
+    mbedtls_mpi_init( &T1 ); mbedtls_mpi_init( &T2 ); mbedtls_mpi_init( &T3 );

     TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 );
     TEST_ASSERT( mbedtls_mpi_read_string( &Y, radix_Y, input_Y ) == 0 );
     TEST_ASSERT( mbedtls_mpi_read_string( &A, radix_A, input_A ) == 0 );
     TEST_ASSERT( mbedtls_mpi_add_mpi( &Z, &X, &Y ) == 0 );
     TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Z, &A ) == 0 );
+    /* Works */
+    TEST_ASSERT( mbedtls_mpi_lset( &Z, 42 ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &Z, &Z, &Z ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_cmp_int( &Z, 84 ) == 0 );
+    /* Works */
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &T1, &Y, &Y ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &T2, &X, &X ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &Z, &T1, &T2 ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &T3, &A, &A ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Z, &T3 ) == 0 );
+    /* Fails */
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &Y, &Y, &Y ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &X, &X, &X ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &Z, &X, &Y ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_add_mpi( &A, &A, &A ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Z, &A ) == 0 );

 exit:
     mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &Z ); mbedtls_mpi_free( &A );
+    mbedtls_mpi_free( &T1 ); mbedtls_mpi_free( &T2 ); mbedtls_mpi_free( &T3 );
 }
 /* END_CASE */

[mpg]

This looks like a bug indeed. We'll investigate and fix it, thanks for your report.

[rainierwolfcastle]

ARM Internal Ref: IOTSSL-512

[yanesca]

I am terribly sorry, but I don't know what the label mirrored means or what the significance of the internal reference number is. Does it mean that this issue will be dealt with internally?

I am just curious, because I already started working on it...

[mpg]

We've just recently started mirroring all github issue to our internal task tracker. This does not change in the slightest the way we handle issue (except it'll make easier for us to track priorities), and there is nothing special about this particular issue.

As all bugs, we intend to fix it as soon as we can get to it :) But if you're already working on a fix, pull requests are always welcome!