Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 6 Nov 2025 at 07:29, <zhaoguohan_salmon@163.com> wrote:
>
> From: GuoHan Zhao <zhaoguohan@kylinos.cn>
>
> Coverity reported a potential out-of-bounds read in rpmb_calc_hmac():
>
> CID 1642869: Out-of-bounds read (OVERRUN)
> Overrunning array of 256 bytes at byte offset 256 by dereferencing
> pointer &frame->data[256].
>
> The issue arises from using &frame->data[RPMB_DATA_LEN] as the source
> pointer for memcpy(). Although computing a one-past-the-end pointer is
> legal, dereferencing it (as memcpy() does) is undefined behavior in C.
>
> Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> ---
>  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 9c86c016cc9d..bc2e9863a534 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,
> +               (const uint8_t *)frame + RPMB_DATA_LEN,
>                 RPMB_HASH_LEN - RPMB_DATA_LEN);
>          offset = lduw_be_p(&frame->address) * RPMB_DATA_LEN + sd_part_offset(sd);
>          do {

What is this code even trying to do ? We define a RPMBDataFrame
which is a packed struct, but now we're randomly memcpying
a lump of data out of the middle of it ??

The start of the struct is
    uint8_t stuff_bytes[RPMB_STUFF_LEN];  // offset 0
    uint8_t key_mac[RPMB_KEY_MAC_LEN];    // offset 196
    uint8_t data[RPMB_DATA_LEN];          // offset 228
    uint8_t nonce[RPMB_NONCE_LEN];        // offset 484

so frame + RPMB_DATA_LEN (256) starts 28 bytes into the data
array; and then we're copying 28 bytes of data?

The existing code (frame->data[RPMB_DATA_LEN]) doesn't make
sense either, as that's a weird way to write frame->nonce,
and the RPMB_NONCE_LEN doesn't have the same length as what
we're copying either.

Can somebody who understands this explain what this code
is intended to be doing ?

thanks
-- PMM