utzig commented on a change in pull request #1845: Fixes to crypto driver and
STM32
URL: https://github.com/apache/mynewt-core/pull/1845#discussion_r293592515
##########
File path: hw/drivers/crypto/crypto_stm32/src/crypto_stm32.c
##########
@@ -119,18 +129,25 @@ stm32_crypto_op(struct crypto_dev *crypto, uint8_t op,
uint16_t algo,
status = HAL_CRYP_AESCTR_Decrypt(&g_hcryp, (uint8_t *)inbuf, len,
outbuf, HAL_MAX_DELAY);
}
+ if (status == HAL_OK) {
+ for (total = 0; total < len; total += AES_BLOCK_LEN) {
Review comment:
This looks interesting, but at least one change would need to be applied
(apart from the `inc >> 8` that should be `inc >>= 8`); the carry has to be
tested in the loop as well, so it would read:
```
for (int i = AES_BLOCK_LEN - 1; (inc != 0 || carry != 0) && i >= 0; --i)
```
But the main issue here is that using `unsigned int` will limit the counter
to `2^32` and the `iv` is 16 bytes instead of 4. Now we could assume that this
always increments up to 32-bit (which is what `tinycrypt` does afair), but the
more correct behaviour is to use all the range, so that after
`0xfffffffffffffffffffffffffffffffe` it correctly incremented to
`0xffffffffffffffffffffffffffffffff` and then rolls back to `0`.
Also the increment in the inner loop will only loop once 90% of the time,
and twice 99% of the time, so the waste is really on the outside loop that
might run one time, or might run 256 times (a 4096 bytes flash sector). This
might deserve an optimization but I would rather have something with no side
effects on the crypto.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
