Re: [PATCH] hw/sd/sdcard: Avoid confusing address calculation in rpmb_calc_hmac

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Jan,

On 14/11/25 22:27, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
>  From the source frame, we initially need to copy out all fields after
> data, thus starting from nonce on. Avoid expressing this indirectly by
> pointing to the end of the data field - which also raised the attention
> of Coverity (out-of-bound read /wrt data).
> 

Resolves: CID 1642869
Fixes: 3acf956ea1a ("hw/sd/sdcard: Handle RPMB MAC field")

> Reported-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Tested, not causing any regression. Please check again if Coverity is
> happy as well. Thanks!
> 
>   hw/sd/sd.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 9c86c016cc..7fdb9195e0 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1161,7 +1161,8 @@ static bool rpmb_calc_hmac(SDState *sd, const RPMBDataFrame *frame,
>   
>           assert(RPMB_HASH_LEN <= sizeof(sd->data));
>   
> -        memcpy((uint8_t *)buf + RPMB_DATA_LEN, &frame->data[RPMB_DATA_LEN],
> +        memcpy((uint8_t *)buf + RPMB_DATA_LEN,
> +               (uint8_t *)frame + offsetof(RPMBDataFrame, nonce),
>                  RPMB_HASH_LEN - RPMB_DATA_LEN);

Having:

#define RPMB_HASH_LEN       (RPMB_DATA_LEN + RPMB_NONCE_LEN)

then

RPMB_HASH_LEN - RPMB_DATA_LEN = RPMB_NONCE_LEN.

So this is equivalent to:

         memcpy((uint8_t *)buf + RPMB_DATA_LEN,
                (uint8_t *)frame + offsetof(RPMBDataFrame, nonce),
                 RPMB_NONCE_LEN);

How is this not equivalent to my previous "broken" suggestion?

         memcpy((uint8_t *)buf + RPMB_DATA_LEN,
                frame->nonce,
                RPMB_NONCE_LEN);

(Yet again late here...)

>           offset = lduw_be_p(&frame->address) * RPMB_DATA_LEN + sd_part_offset(sd);
>           do {