On 14.11.25 14:39, Peter Maydell wrote:
> On Thu, 6 Nov 2025 at 07:29, <[email protected]> wrote:
>>
>> From: GuoHan Zhao <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
