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

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

[zhaoguohan_salmon]

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 {
-- 
2.43.0

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

[Peter Maydell]

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

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

[Jan Kiszka]

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,

Yeah, the originally intended micro-optimization drifted off into
something questionable. But this mitigation is unfortunately broken -
please always check the outcome of a change proposal or, if unsure,
contact the author with your finding first.

>>                 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 ?

What this code does at high-level is documented in the function: We
reconstruct the hashed parts of all read RPMB frames. For that, we start
by filling the tail of RPMBDataFrame from the nonce field onward. Then
we place the data of the individual frames in as well and hash frame by
frame.

I'll send a better version that does not confuse code scanners.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

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

[Philippe Mathieu-Daudé]

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.

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

[Jan Kiszka]

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.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

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

[Philippe Mathieu-Daudé]

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).

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

[Jan Kiszka]

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.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

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

[Philippe Mathieu-Daudé]

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.