lms: Move merkle tree generation to heap allocation #6595

mfischer · 2022-11-12T17:11:08Z

Larger height (e.g. H=20) trees cannot be put on the stack. Allocate memory for them based on need using mbedtls_calloc().

Signed-off-by: Moritz Fischer [email protected]

Description

This is in preparation of adding H=20 support in a separate PR #6584.

Gatekeeper checklist

changelog provided, or not required
backport done, or not required
tests provided, or not required

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

mfischer · 2022-11-15T18:50:59Z

@RcColes @gilles-peskine-arm

library/lms.c

gilles-peskine-arm

Looks mostly good to me, just a few minor things.

library/lms.c

Larger height (e.g. H=20) trees cannot be put on the stack. Allocate memory for them based on need using mbedtls_calloc(). Signed-off-by: Moritz Fischer <[email protected]>

gilles-peskine-arm

Looks good to me.

For future reference, please don't rebase a pull request during review if it isn't needed. (Needed is, for example, to fix a commit message, or because there's a conflict with the base branch.) Work done in response to change requests should go into new commits (broken down into meaningful pieces, not a giant “addressed review comments” commit). Rebasing makes the reviewer's job more difficult because the differences aren't grouped, it's harder to keep track of where you left off, and it leaves an inaccurate history.

mprse

Generally looks good.
Marked only style issues (I don't know if they are important now as we will change code style soon anyway) and left two questions to clarify.

mprse · 2022-12-01T08:08:40Z

library/lms.c

-                &tree[adjacent_node_id],
-                MBEDTLS_LMS_M_NODE_BYTES(ctx->params.type) );
+        memcpy( &path[height * node_bytes],
+                &tree[adjacent_node_id * node_bytes], node_bytes );


Why the second parameter (source address) has been changed?
&tree[adjacent_node_id] -> &tree[adjacent_node_id * node_bytes]

The type of tree changed, now it's a flat unsigned char array.

mprse · 2022-12-01T08:14:13Z

library/lms.c

@@ -499,13 +498,21 @@ static int get_merkle_path( mbedtls_lms_private_t *ctx,
                            unsigned int leaf_node_id,
                            unsigned char *path )
 {
-    unsigned char tree[MERKLE_TREE_NODE_AM_MAX][MBEDTLS_LMS_M_NODE_BYTES_MAX];
+    const size_t node_bytes = MBEDTLS_LMS_M_NODE_BYTES(ctx->params.type);


Suggested change

const size_t node_bytes = MBEDTLS_LMS_M_NODE_BYTES(ctx->params.type);

const size_t node_bytes = MBEDTLS_LMS_M_NODE_BYTES( ctx->params.type );

We're switching to the new style at the end of the month (or maybe first week of January). So let's not be picky about it now.

mprse · 2022-12-01T08:14:29Z

library/lms.c

    unsigned int height;
    int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;

-    ret = calculate_merkle_tree( ctx, ( unsigned char * )tree );
+    tree = mbedtls_calloc( MERKLE_TREE_NODE_AM(ctx->params.type),


Suggested change

tree = mbedtls_calloc( MERKLE_TREE_NODE_AM(ctx->params.type),

tree = mbedtls_calloc( MERKLE_TREE_NODE_AM( ctx->params.type ),

mprse · 2022-12-01T08:15:25Z

library/lms.c

-    ret = calculate_merkle_tree( ctx, ( unsigned char * )tree );
+    tree = mbedtls_calloc( MERKLE_TREE_NODE_AM(ctx->params.type),
+        node_bytes );
+    if ( tree == NULL )


Suggested change

if ( tree == NULL )

if( tree == NULL )

return MBEDTLS_ERR_LMS_ALLOC_FAILED;

mprse · 2022-12-01T08:15:47Z

library/lms.c


        curr_node_id >>=1;
    }

    ret = 0;

 exit:
-    mbedtls_platform_zeroize( tree, sizeof( tree ) );
+    mbedtls_platform_zeroize( tree, node_bytes *
+        MERKLE_TREE_NODE_AM(ctx->params.type) );


Suggested change

MERKLE_TREE_NODE_AM(ctx->params.type) );

MERKLE_TREE_NODE_AM( ctx->params.type ) );

mprse · 2022-12-01T08:17:06Z

library/lms.c

@@ -679,25 +688,33 @@ int mbedtls_lms_calculate_public_key( mbedtls_lms_public_t *ctx,
        return( MBEDTLS_ERR_LMS_BAD_INPUT_DATA );
    }

+    tree = mbedtls_calloc( MERKLE_TREE_NODE_AM(priv_ctx->params.type),


Suggested change

tree = mbedtls_calloc( MERKLE_TREE_NODE_AM(priv_ctx->params.type),

tree = mbedtls_calloc( MERKLE_TREE_NODE_AM( priv_ctx->params.type ),

mprse · 2022-12-01T08:17:42Z

library/lms.c

@@ -679,25 +688,33 @@ int mbedtls_lms_calculate_public_key( mbedtls_lms_public_t *ctx,
        return( MBEDTLS_ERR_LMS_BAD_INPUT_DATA );
    }

+    tree = mbedtls_calloc( MERKLE_TREE_NODE_AM(priv_ctx->params.type),
+        node_bytes );
+    if ( tree == NULL )


Suggested change

if ( tree == NULL )

if ( tree == NULL )

return MBEDTLS_ERR_LMS_ALLOC_FAILED;

mprse · 2022-12-01T08:21:04Z

library/lms.c

    if( ret != 0 )
    {
        goto exit;
    }

    /* Root node is always at position 1, due to 1-based indexing */
-    memcpy( ctx->T_1_pub_key, &tree[1],
-            MBEDTLS_LMS_M_NODE_BYTES(ctx->params.type) );
+    memcpy( ctx->T_1_pub_key, &tree[node_bytes], node_bytes );


Why the second parameter (source address) has been changed?
&tree[1] -> &tree[node_bytes]

The comment above informs why index 1 was used here which is not the case now.

Type of tree changed to a flat unsigned char array. Any suggestions on how to reword the comment?

mprse · 2022-12-01T08:21:29Z

library/lms.c


    ctx->have_public_key = 1;

    ret = 0;

 exit:
-    mbedtls_platform_zeroize( tree, sizeof( tree ) );
+    mbedtls_platform_zeroize( tree, node_bytes *
+        MERKLE_TREE_NODE_AM(priv_ctx->params.type) );


Suggested change

MERKLE_TREE_NODE_AM(priv_ctx->params.type) );

MERKLE_TREE_NODE_AM( priv_ctx->params.type ) );

mprse · 2022-12-01T08:21:42Z

library/lms.c

-    mbedtls_platform_zeroize( tree, sizeof( tree ) );
+    mbedtls_platform_zeroize( tree, node_bytes *
+        MERKLE_TREE_NODE_AM(priv_ctx->params.type) );
+    mbedtls_free ( tree );


Suggested change

mbedtls_free ( tree );

mbedtls_free( tree );

mfischer · 2022-12-03T20:03:39Z

Looks good to me.

For future reference, please don't rebase a pull request during review if it isn't needed. (Needed is, for example, to fix a commit message, or because there's a conflict with the base branch.) Work done in response to change requests should go into new commits (broken down into meaningful pieces, not a giant “addressed review comments” commit). Rebasing makes the reviewer's job more difficult because the differences aren't grouped, it's harder to keep track of where you left off, and it leaves an inaccurate history.

Thanks for the explanation. In terms of process will you guys then squash the commits together when merging?

I'm not super familiar with github flows for this (yet, I'm used to sending patches by email to mailing list).

gilles-peskine-arm · 2022-12-04T11:49:25Z

@mfischer We don't squash. We want to retain the development history in version control, not just the merge history. It's very useful when you want to understand how the code came to be the way it is.

mprse

LGTM.
Style issues ignored.

wernerlewis added enhancement needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Nov 14, 2022

gilles-peskine-arm reviewed Nov 15, 2022

View reviewed changes

library/lms.c Outdated Show resolved Hide resolved

gilles-peskine-arm added the needs-work label Nov 15, 2022

mfischer force-pushed the lms_heap branch from 6822cd9 to 0d3e0ff Compare November 16, 2022 15:39

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work needs-ci Needs to pass CI tests labels Nov 18, 2022

gilles-peskine-arm mentioned this pull request Nov 22, 2022

lms/lmots: Add support for LMS_SHA256_M24_H20 #6584

Draft

3 tasks

gilles-peskine-arm requested changes Nov 22, 2022

View reviewed changes

library/lms.c Outdated Show resolved Hide resolved

library/lms.c Outdated Show resolved Hide resolved

library/lms.c Outdated Show resolved Hide resolved

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Nov 22, 2022

lms: Move merkle tree generation to heap allocation

a6a94ad

Larger height (e.g. H=20) trees cannot be put on the stack. Allocate memory for them based on need using mbedtls_calloc(). Signed-off-by: Moritz Fischer <[email protected]>

mfischer force-pushed the lms_heap branch from 0d3e0ff to a6a94ad Compare November 22, 2022 23:53

mfischer requested a review from gilles-peskine-arm November 23, 2022 16:51

gilles-peskine-arm approved these changes Nov 23, 2022

View reviewed changes

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Nov 23, 2022

mprse self-requested a review December 1, 2022 07:46

mprse reviewed Dec 1, 2022

View reviewed changes

mprse removed the needs-reviewer This PR needs someone to pick it up for review label Dec 1, 2022

mfischer requested a review from mprse December 19, 2022 18:34

mprse approved these changes Dec 21, 2022

View reviewed changes

mprse added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Dec 21, 2022

gilles-peskine-arm mentioned this pull request Dec 23, 2022

Intermittent failure of "Session resume .*, DTLS: openssl server" #5583

Open

gilles-peskine-arm merged commit b402e4b into Mbed-TLS:development Dec 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lms: Move merkle tree generation to heap allocation #6595

lms: Move merkle tree generation to heap allocation #6595

mfischer commented Nov 12, 2022 •

edited by paul-elliott-arm

Loading

mfischer commented Nov 15, 2022

gilles-peskine-arm left a comment

gilles-peskine-arm left a comment

mprse left a comment

mprse Dec 1, 2022

mfischer Dec 1, 2022

mprse Dec 1, 2022

gilles-peskine-arm Dec 1, 2022

mprse Dec 1, 2022

mprse Dec 1, 2022

mprse Dec 1, 2022

mprse Dec 1, 2022

mprse Dec 1, 2022

mprse Dec 1, 2022

mfischer Dec 1, 2022

mprse Dec 1, 2022

mprse Dec 1, 2022

mfischer commented Dec 3, 2022

gilles-peskine-arm commented Dec 4, 2022

mprse left a comment

	const size_t node_bytes = MBEDTLS_LMS_M_NODE_BYTES(ctx->params.type);
	const size_t node_bytes = MBEDTLS_LMS_M_NODE_BYTES( ctx->params.type );

	tree = mbedtls_calloc( MERKLE_TREE_NODE_AM(ctx->params.type),
	tree = mbedtls_calloc( MERKLE_TREE_NODE_AM( ctx->params.type ),

	if ( tree == NULL )
	if( tree == NULL )
	return MBEDTLS_ERR_LMS_ALLOC_FAILED;

	MERKLE_TREE_NODE_AM(ctx->params.type) );
	MERKLE_TREE_NODE_AM( ctx->params.type ) );

	MERKLE_TREE_NODE_AM(priv_ctx->params.type) );
	MERKLE_TREE_NODE_AM( priv_ctx->params.type ) );

lms: Move merkle tree generation to heap allocation #6595

lms: Move merkle tree generation to heap allocation #6595

Conversation

mfischer commented Nov 12, 2022 • edited by paul-elliott-arm Loading

Description

Gatekeeper checklist

Notes for the submitter

mfischer commented Nov 15, 2022

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

mprse 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

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

mfischer commented Dec 3, 2022

gilles-peskine-arm commented Dec 4, 2022

mprse left a comment

Choose a reason for hiding this comment

mfischer commented Nov 12, 2022 •

edited by paul-elliott-arm

Loading