#1064: small MPI cleanups and improvements #1363
Conversation
…ked. 1. MPI small optimizations and improvements: 1.1. introduce `used` to avoid zero skipping loops; 1.2. remove copies in several places; 1.3. reduce number and sizes of memset() calls; 2. Reworked MPI debugging facilities and debug.[ch] 3. Remove secp192r1 as deprecated by RFC8422 5.1.1. 4. Finally get rid of config.h 5. Add MPI unit tests for basic operations to simplify debugging of new changes and adjust ECP and RSA tests. 6. Massive coding style fixes. 7. Proper reference to mbed TLS in licensing.
| @@ -249,8 +250,8 @@ int ttls_asn1_get_sequence_of(unsigned char **p, | |||
| /* Allocate and assign next pointer */ | |||
| if (*p < end) | |||
| { | |||
| cur->next = (ttls_asn1_sequence*)ttls_calloc(1, | |||
| sizeof(ttls_asn1_sequence)); | |||
| cur->next = kmalloc(sizeof(ttls_asn1_sequence), | |||
There was a problem hiding this comment.
Unlike original implementation, this allocation doesn't zeroes the object. Looks suspicious.
There was a problem hiding this comment.
We're good here: cur->next is assigned after the while loop, buf->len is assigned w/o reading in ttls_asn1_get_tag(), buf->tag and buf->p are also assigned in the loop.
| const ttls_mpi *N, | ||
| const ttls_mpi *P, const ttls_mpi *Q, | ||
| const ttls_mpi *D, const ttls_mpi *E) | ||
| const TlsMpi *N, |
There was a problem hiding this comment.
The structure names was autoreplaced and the indentation became broken. The note applies to other functions in this file.
There was a problem hiding this comment.
RSA is to be reworked in #1335 and it has a lot of coding style issues, so I thinks it's better to postpone coding style cleanups until we do something useful with the file. For now we care about ecp* , so let's focus on these files only.
|
|
||
| if (_RR != NULL) | ||
| memcpy(_RR, &RR, sizeof(ttls_mpi)); | ||
| if (!RR || !RR->p) { |
There was a problem hiding this comment.
This part makes no sense anymore.
Previously, one could pass NULL as _RR, and in that case this function did the needed calculations. If _RR wasn't NULL, it was used as RR. Now the function requires a pre-computed RR and won't work correctly if there is NULL. Moreover, in that case ttls_mpi_lset(RR, 1) will be called, which will dereference NULL.
I was wrong, this code is still needed. Although RR can't be NULL, it still can have RR->p == NULL, and in that case this code will calculate the number.
|
|
||
| ttls_mpi_free(&T); | ||
| ttls_rnd(X->p, size); |
There was a problem hiding this comment.
This will fill X->p by size bytes of random data. However, remaining bytes in the last limb will be kept as they were after memory allocation. So it's possible to have random number larger than expected.
| /** | ||
| * Unsigned addition: X = |A| + |B| | ||
| * | ||
| * @A and @B must be different, but either of them can accept the result @X. |
There was a problem hiding this comment.
This may be a neat interface for a general purpose library, but that flexibility usually is not needed, as one knows whenever they do addition X = A + B where all numbers are in different memory blocks, or accumulation A += B. Flexibility makes code slower and more complex. I'd prefer to have two separate functions for the two cases.
| } | ||
| if (!ttls_mpi_cmp_int(B, 1)) { | ||
| if (Q) | ||
| if (ttls_mpi_lset(Q, 1)) |
There was a problem hiding this comment.
It's a division by one, right? Shouldn't Q receive the value of A then?
| M.n++; /* Make room for multiplication by 19 */ | ||
| n = M.used * CIL; | ||
| memcpy(Mp, N->p + P255_WIDTH - 1, n); | ||
| memset(Mp + n, 0, sizeof(Mp) - n); |
There was a problem hiding this comment.
n is number of bytes, but Mp is coerced to unsigned long *, so we may write beyond the Mp here.
All changes are small, except bignum.[ch] where some small pieceis of logic were fully reworked.
1.1. introduce
usedto avoid zero skipping loops;1.2. remove copies in several places;
1.3. reduce number and sizes of memset() calls