[PATCH] hw/sd/sdcard: Avoid confusing address calculation in rpmb_calc_hmac

[PATCH] hw/sd/sdcard: Avoid confusing address calculation in rpmb_calc_hmac

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

From the source frame, we initially need to copy out all fields after
data, thus starting from nonce on. Avoid expressing this indirectly by
pointing to the end of the data field - which also raised the attention
of Coverity (out-of-bound read /wrt data).

Reported-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Tested, not causing any regression. Please check again if Coverity is 
happy as well. Thanks!

 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 9c86c016cc..7fdb9195e0 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,
+               (uint8_t *)frame + offsetof(RPMBDataFrame, nonce),
                RPMB_HASH_LEN - RPMB_DATA_LEN);
         offset = lduw_be_p(&frame->address) * RPMB_DATA_LEN + sd_part_offset(sd);
         do {
-- 
2.51.0

Re: [PATCH] hw/sd/sdcard: Avoid confusing address calculation in rpmb_calc_hmac

[Philippe Mathieu-Daudé]

Hi Jan,

On 14/11/25 22:27, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
>  From the source frame, we initially need to copy out all fields after
> data, thus starting from nonce on. Avoid expressing this indirectly by
> pointing to the end of the data field - which also raised the attention
> of Coverity (out-of-bound read /wrt data).
> 

Resolves: CID 1642869
Fixes: 3acf956ea1a ("hw/sd/sdcard: Handle RPMB MAC field")

> Reported-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Tested, not causing any regression. Please check again if Coverity is
> happy as well. Thanks!
> 
>   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 9c86c016cc..7fdb9195e0 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,
> +               (uint8_t *)frame + offsetof(RPMBDataFrame, nonce),
>                  RPMB_HASH_LEN - RPMB_DATA_LEN);

Having:

#define RPMB_HASH_LEN       (RPMB_DATA_LEN + RPMB_NONCE_LEN)

then

RPMB_HASH_LEN - RPMB_DATA_LEN = RPMB_NONCE_LEN.

So this is equivalent to:

         memcpy((uint8_t *)buf + RPMB_DATA_LEN,
                (uint8_t *)frame + offsetof(RPMBDataFrame, nonce),
                 RPMB_NONCE_LEN);

How is this not equivalent to my previous "broken" suggestion?

         memcpy((uint8_t *)buf + RPMB_DATA_LEN,
                frame->nonce,
                RPMB_NONCE_LEN);

(Yet again late here...)

>           offset = lduw_be_p(&frame->address) * RPMB_DATA_LEN + sd_part_offset(sd);
>           do {

Re: [PATCH] hw/sd/sdcard: Avoid confusing address calculation in rpmb_calc_hmac

[Jan Kiszka]

On 16.11.25 18:43, Philippe Mathieu-Daudé wrote:
> Hi Jan,
> 
> On 14/11/25 22:27, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>>  From the source frame, we initially need to copy out all fields after
>> data, thus starting from nonce on. Avoid expressing this indirectly by
>> pointing to the end of the data field - which also raised the attention
>> of Coverity (out-of-bound read /wrt data).
>>
> 
> Resolves: CID 1642869
> Fixes: 3acf956ea1a ("hw/sd/sdcard: Handle RPMB MAC field")
> 

Feel free to add it. But not that it is not really a bug fix IMHO. It is
a code clarification, output is identical.

>> Reported-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Tested, not causing any regression. Please check again if Coverity is
>> happy as well. Thanks!
>>
>>   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 9c86c016cc..7fdb9195e0 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,
>> +               (uint8_t *)frame + offsetof(RPMBDataFrame, nonce),
>>                  RPMB_HASH_LEN - RPMB_DATA_LEN);
> 
> Having:
> 
> #define RPMB_HASH_LEN       (RPMB_DATA_LEN + RPMB_NONCE_LEN)
> 
> then
> 
> RPMB_HASH_LEN - RPMB_DATA_LEN = RPMB_NONCE_LEN.

This is not correct: 284 - 256 != 16

We hash 284 bytes, that is everything from data field to the end of
RPMBDataFrame.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH] hw/sd/sdcard: Avoid confusing address calculation in rpmb_calc_hmac

[Philippe Mathieu-Daudé]

On 17/11/25 07:09, Jan Kiszka wrote:
> On 16.11.25 18:43, Philippe Mathieu-Daudé wrote:
>> Hi Jan,
>>
>> On 14/11/25 22:27, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>>   From the source frame, we initially need to copy out all fields after
>>> data, thus starting from nonce on. Avoid expressing this indirectly by
>>> pointing to the end of the data field - which also raised the attention
>>> of Coverity (out-of-bound read /wrt data).
>>>
>>
>> Resolves: CID 1642869
>> Fixes: 3acf956ea1a ("hw/sd/sdcard: Handle RPMB MAC field")
>>
> 
> Feel free to add it. But not that it is not really a bug fix IMHO. It is
> a code clarification, output is identical.
> 
>>> Reported-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> Tested, not causing any regression. Please check again if Coverity is
>>> happy as well. Thanks!
>>>
>>>    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 9c86c016cc..7fdb9195e0 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,
>>> +               (uint8_t *)frame + offsetof(RPMBDataFrame, nonce),
>>>                   RPMB_HASH_LEN - RPMB_DATA_LEN);
>>
>> Having:
>>
>> #define RPMB_HASH_LEN       (RPMB_DATA_LEN + RPMB_NONCE_LEN)
>>
>> then
>>
>> RPMB_HASH_LEN - RPMB_DATA_LEN = RPMB_NONCE_LEN.
> 
> This is not correct: 284 - 256 != 16
> 
> We hash 284 bytes, that is everything from data field to the end of
> RPMBDataFrame.

Right. This is why I took so long time before starting to review your
previous series, I was looking for full-focused brain mode. Now than
I could focus again, this is now clear crystal. I'll just add a
"Hash everything from data field to the end of RPMBDataFrame" comment.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

and queued!