* [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac @ 2025-11-06 7:28 zhaoguohan_salmon 2025-11-14 13:39 ` Peter Maydell 0 siblings, 1 reply; 8+ messages in thread From: zhaoguohan_salmon @ 2025-11-06 7:28 UTC (permalink / raw) To: philmd, bmeng.cn; +Cc: qemu-block, qemu-devel, GuoHan Zhao 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac 2025-11-06 7:28 [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac zhaoguohan_salmon @ 2025-11-14 13:39 ` Peter Maydell 2025-11-14 20:10 ` Jan Kiszka 2025-11-14 20:26 ` Philippe Mathieu-Daudé 0 siblings, 2 replies; 8+ messages in thread From: Peter Maydell @ 2025-11-14 13:39 UTC (permalink / raw) To: zhaoguohan_salmon Cc: philmd, bmeng.cn, qemu-block, qemu-devel, GuoHan Zhao, Jan Kiszka 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac 2025-11-14 13:39 ` Peter Maydell @ 2025-11-14 20:10 ` Jan Kiszka 2025-11-14 20:26 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 8+ messages in thread From: Jan Kiszka @ 2025-11-14 20:10 UTC (permalink / raw) To: Peter Maydell, zhaoguohan_salmon Cc: philmd, bmeng.cn, qemu-block, qemu-devel, GuoHan Zhao 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac 2025-11-14 13:39 ` Peter Maydell 2025-11-14 20:10 ` Jan Kiszka @ 2025-11-14 20:26 ` Philippe Mathieu-Daudé 2025-11-14 20:27 ` Jan Kiszka 1 sibling, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2025-11-14 20:26 UTC (permalink / raw) To: Peter Maydell, zhaoguohan_salmon Cc: bmeng.cn, qemu-block, qemu-devel, GuoHan Zhao, Jan Kiszka 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. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac 2025-11-14 20:26 ` Philippe Mathieu-Daudé @ 2025-11-14 20:27 ` Jan Kiszka 2025-11-14 20:34 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 8+ messages in thread From: Jan Kiszka @ 2025-11-14 20:27 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Peter Maydell, zhaoguohan_salmon Cc: bmeng.cn, qemu-block, qemu-devel, GuoHan Zhao 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac 2025-11-14 20:27 ` Jan Kiszka @ 2025-11-14 20:34 ` Philippe Mathieu-Daudé 2025-11-14 20:42 ` Jan Kiszka 0 siblings, 1 reply; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2025-11-14 20:34 UTC (permalink / raw) To: Jan Kiszka, Peter Maydell, zhaoguohan_salmon Cc: bmeng.cn, qemu-block, qemu-devel, GuoHan Zhao 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). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac 2025-11-14 20:34 ` Philippe Mathieu-Daudé @ 2025-11-14 20:42 ` Jan Kiszka 2025-11-14 20:44 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 8+ messages in thread From: Jan Kiszka @ 2025-11-14 20:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Peter Maydell, zhaoguohan_salmon Cc: bmeng.cn, qemu-block, qemu-devel, GuoHan Zhao 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac 2025-11-14 20:42 ` Jan Kiszka @ 2025-11-14 20:44 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2025-11-14 20:44 UTC (permalink / raw) To: Jan Kiszka, Peter Maydell, zhaoguohan_salmon Cc: bmeng.cn, qemu-block, qemu-devel, GuoHan Zhao, Ilias Apalodimas 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-11-14 20:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-06 7:28 [PATCH] hw/sd/sdcard: fix potential out-of-bounds read in rpmb_calc_hmac zhaoguohan_salmon 2025-11-14 13:39 ` Peter Maydell 2025-11-14 20:10 ` Jan Kiszka 2025-11-14 20:26 ` Philippe Mathieu-Daudé 2025-11-14 20:27 ` Jan Kiszka 2025-11-14 20:34 ` Philippe Mathieu-Daudé 2025-11-14 20:42 ` Jan Kiszka 2025-11-14 20:44 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).