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, <[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.
I also have a u-boot test from Ilias I plan to add.
