From: Kane Chen <[email protected]> There is a mismatch between the Aspeed OTP model and the Aspeed SBC model in how the guest-provided address is handled. aspeed_sbc_otp_prog() passes a word-indexed address directly to address_space_write() without converting it to a byte offset, whereas aspeed_otp_write() expects a byte offset and applies an additional shift (otp_addr << 2). This double-shift confusion means that an out-of-range word address can lead to a write beyond the allocated storage.
Fix this by adding bounds checking on the word offset before converting to byte offset and passing to address_space_write(). This matches the existing bounds check in aspeed_sbc_otp_read(). Cc: Kane-Chen-AS <[email protected]> Cc: [email protected] Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model") Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436 Reported-by: Peter Maydell <[email protected]> Signed-off-by: Kane-Chen-AS <[email protected]> Reviewed-by: Peter Maydell <[email protected]> Link: https://lore.kernel.org/qemu-devel/[email protected] [ clg: Kept otp_addr in event logged in aspeed_sbc_otp_prog() ] Signed-off-by: Cédric Le Goater <[email protected]> --- hw/misc/aspeed_sbc.c | 12 ++++++++++-- hw/nvram/aspeed_otp.c | 13 ++++++------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index 065e822e70d9..e5dab1c7bb7c 100644 --- a/hw/misc/aspeed_sbc.c +++ b/hw/misc/aspeed_sbc.c @@ -159,9 +159,17 @@ static bool aspeed_sbc_otp_prog(AspeedSBCState *s, MemTxResult ret; AspeedOTPState *otp = &s->otp; uint32_t value = s->regs[R_CAMP1]; + uint32_t otp_offset = otp_addr << 2; - ret = address_space_write(&otp->as, otp_addr, MEMTXATTRS_UNSPECIFIED, - &value, sizeof(value)); + if (otp_addr >= OTP_TOTAL_DWORD_COUNT) { + qemu_log_mask(LOG_GUEST_ERROR, + "Invalid OTP addr 0x%x\n", + otp_addr); + return false; + } + + ret = address_space_write(&otp->as, otp_offset, MEMTXATTRS_UNSPECIFIED, + &value, sizeof(value)); if (ret != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "Failed to write OTP memory, addr = %x\n", diff --git a/hw/nvram/aspeed_otp.c b/hw/nvram/aspeed_otp.c index a60289000c37..1a9d3841b8d6 100644 --- a/hw/nvram/aspeed_otp.c +++ b/hw/nvram/aspeed_otp.c @@ -57,12 +57,12 @@ static bool valid_program_data(uint32_t otp_addr, return has_programmable_bits != 0; } -static bool program_otpmem_data(void *opaque, uint32_t otp_addr, +static bool program_otpmem_data(void *opaque, hwaddr otp_offset, uint32_t prog_bit, uint32_t *value) { AspeedOTPState *s = opaque; + uint32_t otp_addr = otp_offset >> 2; bool is_odd = otp_addr & 1; - uint32_t otp_offset = otp_addr << 2; memcpy(value, s->storage + otp_offset, sizeof(uint32_t)); @@ -79,26 +79,25 @@ static bool program_otpmem_data(void *opaque, uint32_t otp_addr, return true; } -static void aspeed_otp_write(void *opaque, hwaddr otp_addr, +static void aspeed_otp_write(void *opaque, hwaddr otp_offset, uint64_t val, unsigned size) { AspeedOTPState *s = opaque; - uint32_t otp_offset, value; + uint32_t value; - if (!program_otpmem_data(s, otp_addr, val, &value)) { + if (!program_otpmem_data(s, otp_offset, val, &value)) { qemu_log_mask(LOG_GUEST_ERROR, "%s: Failed to program data, value = %x, bit = %"PRIx64"\n", __func__, value, val); return; } - otp_offset = otp_addr << 2; memcpy(s->storage + otp_offset, &value, size); if (s->blk) { if (blk_pwrite(s->blk, otp_offset, size, &value, 0) < 0) { qemu_log_mask(LOG_GUEST_ERROR, - "%s: Failed to write %x to %x\n", + "%s: Failed to write %x to %"HWADDR_PRIx"\n", __func__, value, otp_offset); return; -- 2.54.0
