Refactor mpi_write_hlp to not be recursive #2214

RonEld · 2018-11-20T12:20:45Z

Description

Refactor mpi_write_hlp() to not be recursive, to fix stack overflows.
Iterate over the mbedtls_mpi division of the radix requested,
until it is zero. Each iteration, put the residue in the next LSB
of the output buffer. Fixes #2190

Status

READY

Requires Backporting

Yes
Which branch?
mbedtls-2.1
mbedtls-2.7

Migrations

NO

Additional comments

Tested on Linux machine, K64F, NRF52840_DK

Todos

Tests
Documentation
Changelog updated
Backported

Steps to test or reproduce

Run the test_suite_mpi on embedded platforms.

hanno-becker · 2018-11-20T12:24:03Z

ChangeLog

@@ -77,6 +77,10 @@ Bugfix
     replacements of standard calloc/free functions through the macros
     MBEDTLS_PLATFORM_CALLOC_MACRO and MBEDTLS_PLATFORM_FREE_MACRO.
     Reported by ole-de and ddhome2006. Fixes #882, #1642 and #1706.
+   * Refactor `mpi_write_hlp()` to not be recursive, to fix stack overflows.


That's a too detailed ChangeLog entry, the first line suffices.

Yeah, I thought about it

I personally like a bit of information regarding the reasoning of a change, but I would phrase it differently. For example:

Reduce stack usage of `mpi_write_hlp()` by eliminating recursion. Fixes #2190.

I like that.

hanno-becker · 2018-11-20T12:24:51Z

library/bignum.c

 {
    int ret;
    mbedtls_mpi_uint r;
+    size_t length = 0;
+    char *p_end = *p + buflen - 1;


Consider failing if buflen == 0 to avoid an underflow.

I considered it, but since the calling function checks the buffer length here, I thought I shouldn't

Yes, for a static function that should be enough. Could you add a comment instead?

hanno-becker · 2018-11-20T12:26:47Z

library/bignum.c

+        MBEDTLS_MPI_CHK( mbedtls_mpi_mod_int( &r, X, radix ) );
+        MBEDTLS_MPI_CHK( mbedtls_mpi_div_int( X, NULL, X, radix ) );
+        /*
+         * Write the residue in the current position, as an  ASCII character.


Minor: Double white space

hanno-becker · 2018-11-20T12:32:10Z

library/bignum.c

+        if( r < 10 )
+            *p_end-- = (char)( r + 0x30 );
+        else
+            *p_end-- = (char)( r + 0x37 );


This is pre-existing, but what is this line supposed to do?

As I wrote in the comment( It took me a while to understand as well):
"Write the residue in the current position, as an ASCII character."
Digits start from 0x30, and A start from 0x41. So, 0x37 + A is 0x41

No need to repeat a comment here. Anyway, what you wrote afterwards clarifies things, thanks.

hanno-becker · 2018-11-20T12:34:55Z

library/bignum.c

-        *(*p)++ = (char)( r + 0x30 );
-    else
-        *(*p)++ = (char)( r + 0x37 );
+    memmove( *p, p_end + 1, length );


Minor improvement: Consider setting p_end = p + buflen in the beginning, and using *--p_end = ... in the loop. This allows to use p_end instead of p_end + 1 here.

I like this approach better

hanno-becker · 2018-11-20T12:36:08Z

library/bignum.c

-    if( mbedtls_mpi_cmp_int( X, 0 ) != 0 )
-        MBEDTLS_MPI_CHK( mpi_write_hlp( X, radix, p ) );
+        length++;
+    } while( mbedtls_mpi_cmp_int( X, 0 ) != 0 && length <= buflen );


Blocker: If length > buflen, we'll leave the loop but nonetheless call memmove() with length as the length parameter.

We should fail if length > buflen.

Same answer as #2214 (comment)
I'll add a comment

No, length is dynamic.

But it starts from 0, and dependent on buflen.
buflen cannot be shorter than n, as checked in https://github.com/ARMmbed/mbedtls/blob/development/library/bignum.c#L552
n should be at most length

In that case, one shouldn't check for length <= buflen. It is highly unintuitive and error-prone to have a buffer length parameter and nonetheless omit bounds checks because one assumes it's large enough. There is technical justification here because of the memmove(), but nonetheless it's dangerous.

Please remove the check length <= buflen from the loop condition and instead check for length < buflen at the beginning of the loop body. This also removes the need for the precondition buflen > 0.

I thought about not adding it at all, but then again, it's always better not to increase the size.
I'll make the change

Yes, very true - if we can, we should be keep the code size down, and if it wasn't for the memmove() I'd be fine with using the fact that we have sufficient space as a precondition. But with the buffer length parameter in place, I think it's misleading to make this assumption.

hanno-becker

Thanks @RonEld, overall the approach looks good to me. I requested some changes, the most relevant ones being a shortening of the ChangeLog entry, and the removal of a potential buffer overflow.

RonEld · 2018-11-20T13:02:27Z

@hanno-arm I addressed your comments

hanno-becker

Thanks for the changes, @RonEld, looks fine apart from the bounds check which should be streamlined as indicated in the comment. This also makes the precondition (and hence the comment) on buflen redundant.

RonEld · 2018-11-20T13:16:35Z

@hanno-arm I fixed according to your comment

hanno-becker

There's an infinite loop if length ever gets to buflen. Suggestion: Check for length >= buflen in the beginning of the loop body and return MBEDTLS_ERR_MPI_BUFFER_TO_SMALL in this case.

RonEld · 2018-11-20T13:25:49Z

@hanno-arm I addressed your comment, but honestly I think it is adding dead code, as in this static function, length will always be smaller than buflen (or equal at the last iteration)

hanno-becker · 2018-11-20T13:32:37Z

library/bignum.c

-    MBEDTLS_MPI_CHK( mbedtls_mpi_div_int( X, NULL, X, radix ) );
+    do
+    {
+        if( length < buflen )


Minor: It would be more readable to write

if( length >= buflen ) return( MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL );

and then do the rest.

Could you fix this, and then we leave the other optimizations for another issue?

hanno-becker · 2018-11-20T13:40:02Z

@RonEld Thanks. I agree that it should never happen, but apart from the reason for including the check I've already given, is it really that obvious that length will never exceed buflen? The caller checks buflen against a shifted version of mbedtls_mpi_get_bitlen(), while the evolution of length in this function depends on iterated calls to mbedtls_mpi_div_int() and mbedtls_mpi_cmp_int(). We rely on all of these functions to work properly if we want to reason that we don't risk an overflow. In my opinion, that's too much risk compared to adding the check. I fully agree that we should reduce code-size whenever we can, but it must not come at the cost of too complex reasoning about the safety of the resulting code.

hanno-becker · 2018-11-20T13:41:24Z

library/bignum.c

@@ -588,7 +604,7 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix,
        if( T.s == -1 )
            T.s = 1;

-        MBEDTLS_MPI_CHK( mpi_write_hlp( &T, radix, &p ) );


Let's save some bytes and RAM here instead, by not using T. Suggestion: Document that mpi_write_hlp() ignores the sign and just pass X instead of the local copy T.

That's actually wrong, because we need a copy in order to successively divide T by the radix.

Probably, but maybe it's better to define T in mpi_write_hlp() instead?

I like @RonEld suggestion

hanno-becker · 2018-11-20T13:43:27Z

library/bignum.c

@@ -588,7 +604,7 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix,
        if( T.s == -1 )


Also, if we want to save code, I think we should call mpi_write_hlp() even for radix 16 and get rid of the radix == 16 branch - it might not be as fast as the hand-coded version, but mbedtls_mpi_write_string() is not time-critical anyway.

We could do that, but I have no strong opinions about it, so this is not a blocker for my approval...

hanno-becker

Requested two code and RAM saving optimizations in the caller mbedtls_write_mpi() that are worth doing along the way. If you insist, @RonEld, that's of course a pre-existing issue and need not be addressed in this PR, but I'd be happy if you could spare the time nonetheless.

RonEld · 2018-11-20T13:49:54Z

@hanno-arm I don't mind doing these changes, I just think they should be tracked in a different github issue

hanno-becker

Looks good to me!

RonEld · 2018-11-20T14:08:06Z

@hanno-arm Thanks for the review!
I created #2216 to track the other two optimization suggestions

hanno-becker · 2018-11-20T14:21:23Z

library/bignum.c

 */
-static int mpi_write_hlp( mbedtls_mpi *X, int radix, char **p )
+static int mpi_write_hlp( mbedtls_mpi *X, int radix, char **p, const size_t buflen )


Minor: This line is too long.

hanno-becker

I only now noticed that there's an overly long line introduced in the PR. Please break it.

hanno-becker · 2018-11-20T14:23:00Z

library/bignum.c

+        if( r < 10 )
+            *(--p_end) = (char)( r + 0x30 );
+        else
+            *(--p_end) = (char)( r + 0x37 );


This is easier to read if we write (char)( 'a' + ( r - 10 ) ).

I think it will be more readable if we change 10 to 0xA

hanno-becker · 2018-11-20T14:23:22Z

library/bignum.c

+         * Write the residue in the current position, as an ASCII character.
+         */
+        if( r < 10 )
+            *(--p_end) = (char)( r + 0x30 );


This is slightly easier to read if we write (char)( '0' + r ).

hanno-becker · 2018-11-20T14:24:43Z

library/bignum.c

-    else
-        *(*p)++ = (char)( r + 0x37 );
+        length++;
+    } while( mbedtls_mpi_cmp_int( X, 0 ) );


Minor: I think usually we are explicit about the != 0. Could you add it?

yes, it's a typo from my previous modification

RonEld · 2018-11-20T14:38:10Z

@Hanno I modified according to your comments

RonEld · 2018-11-20T19:46:11Z

@andresag01 If you haven't started reviewing yet, I will squash the code commits tomorrow, to a single commit, and update the ChangeLog as you requested

andresag01

The changes look good to me, just made a few minor suggestions.

andresag01 · 2018-11-20T19:47:28Z

library/bignum.c

@@ -588,7 +604,7 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix,
        if( T.s == -1 )
            T.s = 1;

-        MBEDTLS_MPI_CHK( mpi_write_hlp( &T, radix, &p ) );


I like @RonEld suggestion

andresag01 · 2018-11-20T19:50:38Z

library/bignum.c

 {
    int ret;
    mbedtls_mpi_uint r;
+    size_t length = 0;
+    char *p_end = *p + buflen;

    if( radix < 2 || radix > 16 )
        return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA );


I think this code is a bit pointless. This function is only a helper called within this compilation module and the caller (i.e. mbedtls_mpi_write_string()) already has the same check, so I think this is effectively dead code.

OK. I guess the original intent of this check was if this helper function would be called from a different function in the future, to check this function's preconditions

andresag01 · 2018-11-20T19:51:59Z

library/bignum.c

@@ -588,7 +604,7 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix,
        if( T.s == -1 )


We could do that, but I have no strong opinions about it, so this is not a blocker for my approval...

andresag01 · 2018-11-20T20:04:32Z

@andresag01 If you haven't started reviewing yet, I will squash the code commits tomorrow, to a single commit, and update the ChangeLog as you requested

Sorry, I had already started reviewing by the time you posted the message...

RonEld · 2018-11-21T12:30:30Z

@andresag01 I have addressed your comments.
Note that #2214 (comment) will be addressed as part of hanno's work on #2216

hanno-becker

LGTM

Refactor `mpi_write_hlp()` to not be recursive, to fix stack overflows. Iterate over the `mbedtls_mpi` division of the radix requested, until it is zero. Each iteration, put the residue in the next LSB of the output buffer. Fixes Mbed-TLS#2190

Update the ChangeLog with the fix.

RonEld · 2018-11-27T08:42:47Z

@andresag01 @hanno-arm I have squashed the PR into two commits. A refernce branch for comparison before the commit is in https://github.com/RonEld/mbedtls/tree/2627_reference
Please review

RonEld · 2018-11-27T08:48:04Z

A backport to mbedtls-2.7 is in #2234

hanno-becker

Looks good to me! Sorry for the long delay.

hanno-becker · 2019-01-14T16:38:34Z

Ping @Patater for gatekeeping.

simonbutcher · 2019-01-27T22:49:39Z

CI is failing on DTLS fragmentations issues:

DTLS proxy: 3d, min handshake, client-initiated renego
DTLS proxy: 3d, FS, ticket
DTLS proxy: 3d, min handshake, server-initiated renego, nbio

The issue appears to be due to timeouts, therefore re-running the CI.

RonEld added bug mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-crypto Crypto primitives and low-level interfaces labels Nov 20, 2018

RonEld changed the title ~~2627~~ Refactor mpi_write_hlp to not be recursive Nov 20, 2018

hanno-becker reviewed Nov 20, 2018

View reviewed changes

hanno-becker suggested changes Nov 20, 2018

View reviewed changes

hanno-becker reviewed Nov 20, 2018

View reviewed changes

hanno-becker suggested changes Nov 20, 2018

View reviewed changes

hanno-becker approved these changes Nov 20, 2018

View reviewed changes

RonEld mentioned this pull request Nov 20, 2018

mbedtls_mpi_write_string() could be reduced to use less RAM. #2216

Closed

hanno-becker reviewed Nov 20, 2018

View reviewed changes

hanno-becker suggested changes Nov 20, 2018

View reviewed changes

hanno-becker reviewed Nov 20, 2018

View reviewed changes

hanno-becker previously approved these changes Nov 20, 2018

View reviewed changes

andresag01 self-requested a review November 20, 2018 17:05

andresag01 suggested changes Nov 20, 2018

View reviewed changes

RonEld dismissed hanno-becker’s stale review via 08021e7 November 21, 2018 12:27

hanno-becker previously approved these changes Nov 21, 2018

View reviewed changes

andresag01 previously approved these changes Nov 26, 2018

View reviewed changes

Ron Eldor added 2 commits November 27, 2018 10:34

Refactor mpi_write_hlp to not be recursive

a16fa29

Refactor `mpi_write_hlp()` to not be recursive, to fix stack overflows. Iterate over the `mbedtls_mpi` division of the radix requested, until it is zero. Each iteration, put the residue in the next LSB of the output buffer. Fixes Mbed-TLS#2190

Update ChangeLog

8a6917d

Update the ChangeLog with the fix.

RonEld dismissed stale reviews from andresag01 and hanno-becker via 8a6917d November 27, 2018 08:41

RonEld force-pushed the 2627 branch from 08021e7 to 8a6917d Compare November 27, 2018 08:41

RonEld mentioned this pull request Nov 27, 2018

Backport 2.7: Refactor mpi_write_hlp to not be recursive #2234

Merged

andresag01 approved these changes Nov 27, 2018

View reviewed changes

hanno-becker approved these changes Jan 14, 2019

View reviewed changes

hanno-becker removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels Jan 14, 2019

simonbutcher added the needs-ci Needs to pass CI tests label Jan 27, 2019

Patater added the approved Design and code approved - may be waiting for CI or backports label Jan 30, 2019

Patater merged commit 8a6917d into Mbed-TLS:development Jan 31, 2019

		@@ -588,7 +604,7 @@ int mbedtls_mpi_write_string( const mbedtls_mpi *X, int radix,
		if( T.s == -1 )

Refactor mpi_write_hlp to not be recursive #2214

Refactor mpi_write_hlp to not be recursive #2214

Conversation

RonEld commented Nov 20, 2018

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

hanno-becker Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker left a comment

Choose a reason for hiding this comment

RonEld commented Nov 20, 2018

hanno-becker left a comment • edited Loading

Choose a reason for hiding this comment

RonEld commented Nov 20, 2018

hanno-becker left a comment • edited Loading

Choose a reason for hiding this comment

RonEld commented Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker commented Nov 20, 2018 • edited Loading

hanno-becker Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker left a comment

Choose a reason for hiding this comment

RonEld commented Nov 20, 2018

hanno-becker left a comment

Choose a reason for hiding this comment

RonEld commented Nov 20, 2018

Choose a reason for hiding this comment

hanno-becker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Nov 20, 2018

RonEld commented Nov 20, 2018

andresag01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Nov 20, 2018

RonEld commented Nov 21, 2018

hanno-becker left a comment

Choose a reason for hiding this comment

hanno-becker Nov 20, 2018 •

edited

Loading

RonEld Nov 20, 2018 •

edited

Loading

hanno-becker left a comment •

edited

Loading

hanno-becker left a comment •

edited

Loading

RonEld commented Nov 20, 2018 •

edited

Loading

hanno-becker commented Nov 20, 2018 •

edited

Loading

hanno-becker Nov 20, 2018 •

edited

Loading

hanno-becker Nov 20, 2018 •

edited

Loading