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

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

On 14/11/25 21:27, Jan Kiszka wrote:
> On 14.11.25 21:26, Philippe Mathieu-Daudé wrote:
>> 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);
> 
> Also broken.

Sorry, long day :)

We really should add a functional test covering RPMB (I'd have
run it mechanically before posting my reply).