Have mbedtls_mpi_core_exp_mod() take a temporary instead of allocating memory #6733
Conversation
…ry form Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>
Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>
Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>
…g memory Last PR needed for Mbed-TLS#6293 Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>
| const mbedtls_mpi_uint *N, size_t AN_limbs, | ||
| const mbedtls_mpi_uint *E, size_t E_limbs, | ||
| const mbedtls_mpi_uint *RR ); | ||
| size_t mbedtls_mpi_core_exp_mod_working_limbs( size_t AN_limbs, size_t E_limbs ); |
There was a problem hiding this comment.
I am a bit concerned by this helper function. I appreciate that we are sensible and only using it during testing, and I can see how helpfull it is to abstract memory allocation math, from the higher layers.
But my concern is that it is a function that people will invoke right before allocating a memory block.
This is a core method so we should not be adding extra checking but we could document that there is not overflow detection, especially for AN_limbs < (SIZE_MAX - 1) / 2.
Please note that SIZE_MAX is a moving target since it is implementation defined.
There was a problem hiding this comment.
The parameters are sizes of existing bignum objects. The size of bignum objects should be limited by MBEDTLS_MPI_MAX_LIMBS, which is small enough for the calculation not to overflow. So this function doesn't really need special warnings.
There was a problem hiding this comment.
A static assertion on MBEDTLS_MPI_MAX_LIMBS could be useful, in the unlikely case users set it to an unreasonable value, or in case users want to use the library on a platform with 16-bit size_t (which we do not officially support, and such users would be very unlikely to enlarge MBEDTLS_MPI_MAX_LIMBS). But that's only a defense against a configuration mistake that isn't particularly likely. I think anything more than that is overkill.
There was a problem hiding this comment.
So how about if I add
* \warning No error checking is performed on the value of the inputs,
* so "caveat caller". With the current algorithm, overflow can
* occur (depends on the value of \p E_limbs, too) if
* `AN_limbs > (SIZE_MAX / sizeof(mbedtls_mpi_uint) - 1) / 67`.
* In practice, with MPIs having a maximum of MBEDTLS_MPI_MAX_LIMBS
* limbs, we are several orders of magnitude away from this.
* Further, the number of limbs will usually be determined by
* the modulus, which will normally be under our control.
to the documentation of mbedtls_mpi_core_exp_mod_working_limbs()?
There was a problem hiding this comment.
Reviewing latest commit 0a0dded
Looks good, with a minor documentation request
| const mbedtls_mpi_uint *N, size_t AN_limbs, | ||
| const mbedtls_mpi_uint *E, size_t E_limbs, | ||
| const mbedtls_mpi_uint *RR ); | ||
| size_t mbedtls_mpi_core_exp_mod_working_limbs( size_t AN_limbs, size_t E_limbs ); |
There was a problem hiding this comment.
The parameters are sizes of existing bignum objects. The size of bignum objects should be limited by MBEDTLS_MPI_MAX_LIMBS, which is small enough for the calculation not to overflow. So this function doesn't really need special warnings.
|
CI green apart from OpenCI |
This is the last PR needed for #6293
Based on top of #6731 (which now has two approvals, just waiting for CI to complete before it can go in)
The new content of this PR is the final commit
Gatekeeper checklist