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:42, Jan Kiszka wrote:
> On 14.11.25 21:34, Philippe Mathieu-Daudé wrote:
>> 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 :)
>>
> 
> Yeah, me too :)
> 
>> We really should add a functional test covering RPMB (I'd have
>> run it mechanically before posting my reply).
>>
> 
> I don't disagree. I have to re-run my full image test for that. A qemu
> test just needs a bit time to work it out.

I also have a u-boot test from Ilias I plan to add.