Skip to content

Conversation

@mschulz-at-hilscher
Copy link
Contributor

Description

Added a benchmark for the LMS post-quantum-safe stateful-hash public-key signature scheme.
As the private key is limited to a number of signing operations resp. number of one-time private keys, the benchmark stops as soon as it runs out of keys.

I also ran scripts/code_style.py to uncrustify the benchmark.c. Those changes are in the second commit, while the new code is in the first commit.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog not required
  • backport not required
  • tests not required

Signed-off-by: Matthias Schulz <mschulz@hilscher.com>
@gowthamsk-arm gowthamsk-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Nov 10, 2023
@mschulz-at-hilscher
Copy link
Contributor Author

mschulz-at-hilscher commented Nov 13, 2023

Done now. Just kept for reference:

Dear reviewers,
when I run the code_style.py script on programs/test/benchmark.c it succeeds without problems, but somehow the PR tests fail when it comes to code style checking. What am I doing wrong?

./scripts/code_style.py programs/test/benchmark.c
Warning: Using unsupported Uncrustify version 'Uncrustify-0.77.1-127-566eb1a93'
Note: The only supported version is 0.75.1
Checked 1 files, style ok.

@gilles-peskine-arm
Copy link
Contributor

Different versions of uncrustify produce different results. You need to use 0.75.1.

We wrote that warning message before 0.76 came out. Now that it's out and we know it produces different results in our code base, I guess we should turn the warning into an error.

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Nov 13, 2023
3 tasks
@mschulz-at-hilscher
Copy link
Contributor Author

Could you trigger the CI tests again. They should pass now.

@mschulz-at-hilscher
Copy link
Contributor Author

It seems like there is no progress in this PR. Is there anything else you need from me to get the changes merged?

@gowthamsk-arm
Copy link
Contributor

We will be looking at it.

@mschulz-at-hilscher
Copy link
Contributor Author

Happy new year. Any chance to get the PR merged soon?

@waleed-elmelegy-arm
Copy link
Contributor

waleed-elmelegy-arm commented Jan 2, 2024

CI failure because of code style with the below error:

[2023-11-28T12:20:15.830Z] --- programs/test/benchmark.c	2023-11-28 12:19:33.121686881 +0000

[2023-11-28T12:20:15.830Z] +++ programs/test/benchmark.c.uncrustify	2023-11-28 12:20:14.065525618 +0000

[2023-11-28T12:20:15.830Z] @@ -1331,7 +1331,8 @@

[2023-11-28T12:20:15.830Z]      if (todo.lms) {

[2023-11-28T12:20:15.830Z]          mbedtls_lms_public_t pub_ctx;

[2023-11-28T12:20:15.830Z]          mbedtls_lms_private_t priv_ctx;

[2023-11-28T12:20:15.830Z] -        unsigned char sig[MBEDTLS_LMS_SIG_LEN(MBEDTLS_LMS_SHA256_M32_H10, MBEDTLS_LMOTS_SHA256_N32_W8)];

[2023-11-28T12:20:15.830Z] +        unsigned char sig[MBEDTLS_LMS_SIG_LEN(MBEDTLS_LMS_SHA256_M32_H10,

[2023-11-28T12:20:15.831Z] +                                              MBEDTLS_LMOTS_SHA256_N32_W8)];

[2023-11-28T12:20:15.831Z]  

[2023-11-28T12:20:15.831Z]          mbedtls_snprintf(title, sizeof(title), "LMS");

[2023-11-28T12:20:15.831Z]  

[2023-11-28T12:20:15.831Z] @@ -1339,23 +1340,25 @@

[2023-11-28T12:20:15.831Z]          mbedtls_lms_private_init(&priv_ctx);

[2023-11-28T12:20:15.831Z]  

[2023-11-28T12:20:15.831Z]          mbedtls_lms_generate_private_key(&priv_ctx, MBEDTLS_LMS_SHA256_M32_H10,

[2023-11-28T12:20:15.831Z] -                                                    MBEDTLS_LMOTS_SHA256_N32_W8,

[2023-11-28T12:20:15.831Z] -                                                    myrand, NULL,

[2023-11-28T12:20:15.831Z] -                                                    buf, BUFSIZE);

[2023-11-28T12:20:15.831Z] +                                         MBEDTLS_LMOTS_SHA256_N32_W8,

[2023-11-28T12:20:15.831Z] +                                         myrand, NULL,

[2023-11-28T12:20:15.831Z] +                                         buf, BUFSIZE);

[2023-11-28T12:20:15.831Z]  

[2023-11-28T12:20:15.831Z]          mbedtls_lms_calculate_public_key(&pub_ctx, &priv_ctx);

[2023-11-28T12:20:15.831Z]  

[2023-11-28T12:20:15.831Z]          TIME_PUBLIC(title, "sign",

[2023-11-28T12:20:15.831Z] -            ret = mbedtls_lms_sign(&priv_ctx, myrand, NULL, buf, BUFSIZE, sig, sizeof(sig), NULL);

[2023-11-28T12:20:15.831Z] -            if (ret == MBEDTLS_ERR_LMS_OUT_OF_PRIVATE_KEYS) {

[2023-11-28T12:20:15.831Z] -                ret = 0;

[2023-11-28T12:20:15.831Z] -                break;

[2023-11-28T12:20:15.831Z] -            }

[2023-11-28T12:20:15.831Z] -        );

[2023-11-28T12:20:15.831Z] +                    ret =

[2023-11-28T12:20:15.831Z] +                        mbedtls_lms_sign(&priv_ctx, myrand, NULL, buf, BUFSIZE, sig, sizeof(sig),

[2023-11-28T12:20:15.831Z] +                                         NULL);

[2023-11-28T12:20:15.831Z] +                    if (ret == MBEDTLS_ERR_LMS_OUT_OF_PRIVATE_KEYS) {

[2023-11-28T12:20:15.831Z] +            ret = 0;

[2023-11-28T12:20:15.831Z] +            break;

[2023-11-28T12:20:15.831Z] +        }

[2023-11-28T12:20:15.831Z] +                    );

[2023-11-28T12:20:15.831Z]  

[2023-11-28T12:20:15.831Z]          TIME_PUBLIC(title, "verify",

[2023-11-28T12:20:15.831Z] -            ret = mbedtls_lms_verify(&pub_ctx, buf, BUFSIZE, sig, sizeof(sig));

[2023-11-28T12:20:15.831Z] -        );

[2023-11-28T12:20:15.831Z] +                    ret = mbedtls_lms_verify(&pub_ctx, buf, BUFSIZE, sig, sizeof(sig));

[2023-11-28T12:20:15.831Z] +                    );

[2023-11-28T12:20:15.831Z]  

[2023-11-28T12:20:15.831Z]          mbedtls_lms_public_free(&pub_ctx);

[2023-11-28T12:20:15.831Z]          mbedtls_lms_private_free(&priv_ctx);

[2023-11-28T12:21:37.299Z] programs/test/benchmark.c changed - code style is incorrect.

[2023-11-28T12:21:37.299Z] ^^^^check_code_style: Check C code style: ./scripts/code_style.py -> 1^^^^
``` `

to reproduce run:
`./scripts/code_style.py `

@tom-daubney-arm
Copy link
Contributor

Hey @mschulz-at-hilscher , thanks for the contribution. Are you still keen to get this merged? I have some bandwidth to review it so we can get it over the line.

You have CI failures coming from our uncrustify automated code style checks. I took a look at it and can see 4 complaints coming from uncrustify; 3 of them look legitimate, but one of them looks like uncrustify has suffered a glitch.

In this case the glitch has occurred on line 1348 in programs/test/benchmark.c where the ret = 0 statement and the 2 lines below it have been incorrectly aligned.

I think the easiest way to fix this is to wrap the code that uncrustify flags as a false positive with an /* *INDENT-OFF* */ and /* *INDENT-ON* */ comment. This will get uncrustify to ignore this particular bit of code. This is what I mean:

        /* *INDENT-OFF* */
        TIME_PUBLIC(title, "sign",
            ret = mbedtls_lms_sign(&priv_ctx, myrand, NULL, buf, BUFSIZE, sig, sizeof(sig), NULL);
            if (ret == MBEDTLS_ERR_LMS_OUT_OF_PRIVATE_KEYS) {
                ret = 0;
                break;
            }
        );
        /* *INDENT-ON* */

Once that is done, just run ./scripts/code_style.py -f locally from the repository root. This will get uncrustify to fix any flaws it finds, which in this case will be the 3 legitimate code style issues, and it will ignore the false positive due to the commenting I suggest above. After uncrustify runs you can just commit those changes and push the PR.

@tom-daubney-arm tom-daubney-arm self-requested a review January 23, 2024 16:41
@mschulz-at-hilscher
Copy link
Contributor Author

Hi Tom,
I will work on it after my performance extensions for GCM.

@tom-daubney-arm
Copy link
Contributor

Hi @mschulz-at-hilscher , is there any interest still from you to fix the code style issue?

@mschulz-at-hilscher
Copy link
Contributor Author

Hi @mschulz-at-hilscher , is there any interest still from you to fix the code style issue?

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci Needs to pass CI tests 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 priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)

Projects

Status: In Development

Development

Successfully merging this pull request may close these issues.

5 participants