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

mbedtls_mpi_exp_mod allow output to alias inputs #6587

Closed

Conversation

gstrauss
Copy link
Contributor

Description

mbedtls_mpi_exp_mod allow output to alias inputs

follow-up to #6552
and independent but related to #6585 for mbedtls_mpi_div_mpi()

Gatekeeper checklist

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

@tom-cosgrove-arm
Copy link
Contributor

Since you're expanding the definition of the function to allow aliasing, could you add tests for these? If you look at the bignum work we're doing, you'll see examples of this being done

@gstrauss
Copy link
Contributor Author

This and related PR #6585 aim to fix a few issues first acknowledged in #1105 from Sep 2017, over 5 years ago, which have been tripped over by me and others (often at a huge time cost to track down) and for which I recently submitted a PR to at least add comments (#6552) (and that PR was triaged and accepted quickly).

Since you're expanding the definition of the function to allow aliasing, could you add tests for these? If you look at the bignum work we're doing, you'll see examples of this being done

For writing tests to verify that the function works with aliasing, are you looking for a few smoke tests, or are you looking for extensive testing of a matrix of parameter dimensions (which would probably be better tested with fuzzing)?

TBH, (and not aimed at you) I might be more inspired if the mbedtls team were a bit more consistent with handling PRs. ...I would also be more inclined to write additional tests in a follow-up PR.

https://github.com/Mbed-TLS/mbedtls/pulls/gstrauss has at least one PR that is small and has had one approval for 9+ weeks (#6072) and is marked "priority-medium", with no second approver. There is another PR marked "priority-medium" that is many months old without any feedback (#6003). That PR was submitted 1 July and even though marked "priority-medium", has not been touched beyond initial labelling since 4 July. This suggests "priority-medium" is equivalent to "priority-low" is equivalent to no-priority. To be clear, I do understand time limitations. My criticism is aimed at the process failures. The mbedtls CI systems are drowning while rebuilding many PRs that the team is not looking at.

@tom-cosgrove-arm
Copy link
Contributor

are you looking for a few smoke tests, or are you looking for extensive testing of a matrix of parameter dimensions

Here I think a few smoke tests would be fine

@gstrauss
Copy link
Contributor Author

are you looking for a few smoke tests, or are you looking for extensive testing of a matrix of parameter dimensions

Here I think a few smoke tests would be fine

Ok. I'll add some this weekend.

@wernerlewis wernerlewis 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-ci Needs to pass CI tests labels Nov 11, 2022
Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
@@ -1061,6 +1061,39 @@ exit:
}
/* END_CASE */

/* BEGIN_CASE */
void mpi_exp_mod_aliasing( int result )
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as https://github.com/Mbed-TLS/mbedtls/pull/6588/files#r1021216383: please don't introduce this new test structure, the established one is better.

@gilles-peskine-arm
Copy link
Contributor

Thank you for your contribution to Mbed TLS. Unfortunately, it partially conflicts the direction in which the project is moving, so we are not going to move forward with it.

We are currently rewriting the bignum module to have a new interface which does not use malloc internally and which does not support negative numbers. The current bignum.h will be deprecated once this rewrite is over, and will be removed in a future major version of Mbed TLS.

As a consequence, we've decided not to accept any more library improvements that are only relevant to the current bignum interface. This will give us more bandwidth to review code with a longer shelf life. Documentation improvements and bug fixes are still welcome.

I am closing this pull request since it's about improving aliasing capabilities in bignum. Any limitations to aliasing in the current bignum interface are going to be documented if we identify them, but not lifted.

Roadmap Board for Mbed TLS automation moved this from To Do to Done Nov 14, 2022
@gilles-peskine-arm
Copy link
Contributor

Regarding priority of reviews: the priority label is an indication, but there are other concerns. As a team, we prioritize bug fixes over new features and performance improvements. As individuals, we tend to prioritize areas of the code we're comfortable with. Generally, small pull requests have an easier time because there's a better chance that someone will have a spare half-hour and review them, versus a large pull request that requires a multi-day commitment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement 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 size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants