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, <[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, >>>>> 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
