PSA PAKE: Check input_length against PSA_PAKE_INPUT_SIZE() in psa_pake_input #7331
Conversation
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
mprse
added
needs-review
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
mprse
added
enhancement
component-psa
AndrzejKurek
removed
needs-review
library/psa_crypto.c
Outdated
| @@ -7920,7 +7925,8 @@ psa_status_t psa_pake_input( | |||
| goto exit; | |||
| } | |||
|
|
|||
| if (input_length == 0 || input_length > PSA_PAKE_INPUT_MAX_SIZE) { | |||
|
|
|||
There was a problem hiding this comment.
Minor: double newline
| @@ -2106,6 +2106,8 @@ struct psa_pake_operation_s { | |||
| unsigned int MBEDTLS_PRIVATE(id); | |||
| /* Algorithm of the PAKE operation */ | |||
| psa_algorithm_t MBEDTLS_PRIVATE(alg); | |||
| /* A primitive of type compatible with algorithm */ | |||
| psa_pake_primitive_t MBEDTLS_PRIVATE(primitive); | |||
There was a problem hiding this comment.
One thing that I would suggest to consider is moving the new structure field to the end to conserve backward ABI compatibility.
There was a problem hiding this comment.
I'm not sure about moving this field to the end of the structure as locating it with id and alg values makes more sense. Then we have unions.
There was a problem hiding this comment.
Maybe it's a question to one of our gatekeepers? Whether we prefer backwards ABI compatibility or better readability.
There was a problem hiding this comment.
We end up breaking ABI compatibility in pretty much every release these days. So one more ABI break doesn't matter. (Except in LTS, there we want to preserve the ABI as well as the API.)
Beyond readability for grouping, there are other considerations for the ordering of struct fields. I haven't checked what might apply here.
- Avoid holes due to alignment. E.g. prefer
{char; char; short;}or{short; char; char}to{char; short; char;}since the first two typically fill 4 bytes but the last one typically needs 6 bytes, with two padding bytes. - For long structures: on Cortex-M, it takes less code to access the first 128 elements, so it's better to put often-accessed byte fields within the first 128 bytes, 2-byte fields within 256 bytes, 4-byte fields within 512 bytes. Here, “often-accessed” is about the number of places in the code that access the field, not about how frequent the accesses are at runtime. Of course the very first field is even cheaper. See Report on structure fields #5234
- For long structures: if some fields are often accessed at the same time, it helps if they're on the same cache line. The size of a cache line depends on the architecture, of course, but what this means is that it helps if they're close together.
- If some parts of a structure depend on compilation option, it's usually easier to follow things in a debugger if all the common parts are at the beginning.
|
Apart from some suggestions to consider before marking this PR as approved, please also update the PR description of backport, tests, and changelog fields. |
Tests are provided but I'm not sure if we need changelog for this change. I think backport is not needed. |
Sure, what I mean is to update the description at the top, so that the gatekeepers can see a justification :) |
davidhorstmann-arm
added
the
approved
Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>
mprse
dismissed stale reviews from davidhorstmann-arm and AndrzejKurek
via
7921a03
|
davidhorstmann-arm
added
needs-review
davidhorstmann-arm
added
approved
Description
Resolves #7226
Gatekeeper checklist
Tests provided.
Backport and changelog not needed.
Notes for the submitter
Please refer to the contributing guidelines, especially the
checklist for PR contributors.