Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport 3.6] Fix doc on GCM API #9762

Open
wants to merge 6 commits into
base: mbedtls-3.6
Choose a base branch
from

Conversation

Harry-Ramsey
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey commented Nov 7, 2024

Description

This is a backport of #7977.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

Notes for the submitter

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

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

This commit updates GCM buffer overlap documentation to explicitly
support overlapping encryption/decryption buffers for GCM.

Signed-off-by: Harry Ramsey <[email protected]>
This commit adds test cases for input and output buffer overlap. The
data for the test cases is a duplicate of existing encrypt/decrypt test
cases.

The two test functions gcm_<encrypt/decrypt>_input_output_buffer_overlap
are modified to use a single malloc buffer rounded to the nearest
128-bits/16-bytes for input and output.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey Harry-Ramsey changed the base branch from development to mbedtls-3.6 November 7, 2024 09:30
@Harry-Ramsey Harry-Ramsey mentioned this pull request Nov 7, 2024
4 tasks
if (buffer_len % 16 != 0) {
buffer_len += (16 - (buffer_len % 16));
}
TEST_CALLOC(buffer, buffer_len);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding test faliures, can TEST_CALLOC not be used in 3.6?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TEST_CALLOC is available and (should be) identical in all supported branches.

You don't need if NULL goto exit: that's built into the macro. Furthermore, buffer will actually be null on success when buffer_len == 0. That may be the cause of the CI failures? It would be the same in development though.

@Harry-Ramsey Harry-Ramsey self-assigned this Nov 7, 2024
@Harry-Ramsey Harry-Ramsey added needs-ci Needs to pass CI tests enhancement priority-medium Medium priority - this can be reviewed as time permits labels Nov 7, 2024
This commit fixes issues with TEST_CALLOC in GCM buffer overlap tests
cases.

Signed-off-by: Harry Ramsey <[email protected]>
This commit fixes an issue with check_names failing due to not being
defined as a macro in Mbed TLS. This is instead defined by alternative
implementations of MBEDTLS_GCM.

Signed-off-by: Harry Ramsey <[email protected]>
This commit fixes an issue in the GCM shared buffer test case where
input data could be of length 0 and an adequate buffer was not
allocated.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey Harry-Ramsey added needs-review Every commit must be reviewed by at least two team members, size-xs Estimated task size: extra small (a few hours at most) and removed needs-ci Needs to pass CI tests labels Nov 13, 2024
@eleuzi01 eleuzi01 self-requested a review November 13, 2024 15:28
eleuzi01
eleuzi01 previously approved these changes Nov 13, 2024
Copy link
Contributor

@eleuzi01 eleuzi01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from the no-check-names commit. This should not be needed on this branch, did you cherry-pick from development when backporting?

This commit fixes documentation specifying buffer overlap on GCM
decryption functions.

Signed-off-by: Harry Ramsey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs-review Every commit must be reviewed by at least two team members, priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants