On Fri, 4 Jul 2025 at 23:32, Jackson Donaldson <jackson88...@gmail.com> wrote:
>
> This commit implements AES for the MAX78000
>
> Signed-off-by: Jackson Donaldson <jc...@duck.com>
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>

Hi; now this is upstream Coverity Scan noticed a possible
issue in this function (CID 1612247):

> +static void max78000_aes_do_crypto(Max78000AesState *s)
> +{
> +    int keylen = 256;
> +    uint8_t *keydata = s->key;
> +    if ((s->ctrl & KEY_SIZE) == 0) {
> +        keylen = 128;
> +        keydata += 16;
> +    } else if ((s->ctrl & KEY_SIZE) == 1 << 6) {
> +        keylen = 192;
> +        keydata += 8;
> +    }
> +
> +    AES_KEY key;
> +    if ((s->ctrl & TYPE) == 0) {
> +        AES_set_encrypt_key(keydata, keylen, &key);
> +        AES_set_decrypt_key(keydata, keylen, &s->internal_key);
> +        AES_encrypt(s->data, s->result, &key);

Here we call AES_set_encrypt_key() and AES_set_decrypt_key()
before calling AES_encrypt()...

> +        s->result_index = 16;
> +    } else if ((s->ctrl & TYPE) == 1 << 8) {
> +        AES_set_decrypt_key(keydata, keylen, &key);
> +        AES_set_decrypt_key(keydata, keylen, &s->internal_key);
> +        AES_decrypt(s->data, s->result, &key);

...here we call AES_set_decrypt_key() twice before
calling AES_decrypt(). This looks a bit odd: should we either
(a) call both AES_set_decrypt_key() and AES_set_encrypt_key()
in each half of the if(), or (b) call AES_set_encyrypt_key()
twice in the AES_encrypt() code path ?

(Coverity is sometimes wrong, as it's only using a heuristic
here, so the other option is "the code as written is correct",
but in that case a comment might be helpful for human readers.)

thanks
-- PMM

Reply via email to