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. Jan -- Siemens AG, Foundational Technologies Linux Expert Center
