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

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

Hi Zhao, Peter,

On 14/11/25 14:39, Peter Maydell wrote:
> 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.

Indeed.

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

We hash the frame data[] + nonce[], and work on the card block buffer
('buf'), filling it before hashing.

This change should clarify:

-- >8 --
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9c86c016cc9..e60311e49a6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -125 +125,2 @@ typedef struct SDProto {
-#define RPMB_HASH_LEN       284
+
+#define RPMB_HASH_LEN       (RPMB_DATA_LEN + RPMB_NONCE_LEN)
@@ -1164,2 +1165 @@ static bool rpmb_calc_hmac(SDState *sd, const 
RPMBDataFrame *frame,
-        memcpy((uint8_t *)buf + RPMB_DATA_LEN, &frame->data[RPMB_DATA_LEN],
-               RPMB_HASH_LEN - RPMB_DATA_LEN);
+        memcpy((uint8_t *)buf + RPMB_DATA_LEN, frame->nonce, 
RPMB_NONCE_LEN);
---

Regards,

Phil.