[PATCH v1 0/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[PATCH v1 0/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Kane Chen]

This series fixes a bounds issue in Aspeed OTP programming through the
Secure Boot Controller path.

The guest-provided OTP address is word-indexed in the SBC model, but
the OTP device write path operates on byte offsets. Passing the value
through without validation/conversion could lead to out-of-range writes.

The patch adds bounds checking in aspeed_sbc_otp_prog() before
converting the address to a byte offset, and aligns the OTP write
helper interfaces with byte-offset semantics.

The patch has been validated by a functional test and by the boundary
test documented at:
https://gitlab.com/qemu-project/qemu/-/work_items/3436

Kane-Chen-AS (1):
  hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

 hw/misc/aspeed_sbc.c  | 14 +++++++++++---
 hw/nvram/aspeed_otp.c | 13 ++++++-------
 2 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Kane Chen]

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 <kane_chen@aspeedtech.com>
Cc: qemu-stable@nongnu.org
Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
 hw/misc/aspeed_sbc.c  | 14 +++++++++++---
 hw/nvram/aspeed_otp.c | 13 ++++++-------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index 065e822e70..8b74dca13c 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -159,13 +159,21 @@ 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",
-                      otp_addr);
+                      otp_offset);
         return false;
     }
 
diff --git a/hw/nvram/aspeed_otp.c b/hw/nvram/aspeed_otp.c
index a60289000c..1a9d3841b8 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.43.0

Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Peter Maydell]

On Tue, 28 Apr 2026 at 06:53, Kane Chen <kane_chen@aspeedtech.com> wrote:
>
> 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 <kane_chen@aspeedtech.com>
> Cc: qemu-stable@nongnu.org
> Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>  hw/misc/aspeed_sbc.c  | 14 +++++++++++---
>  hw/nvram/aspeed_otp.c | 13 ++++++-------
>  2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> index 065e822e70..8b74dca13c 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -159,13 +159,21 @@ 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",
> -                      otp_addr);
> +                      otp_offset);

I would stick to reporting otp_addr here:
 - it's the value the guest actually writes
 - it matches the logging for "invalid OTP addr above"
 - it matches what aspeed_sbc_otp_read() logs in the equivalent case

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

回覆: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Kane Chen]

[-- Attachment #1: Type: text/plain, Size: 3556 bytes --]

Hi Peter,

Thanks for your review and feedback. I will update the logging accordingly and submit a v2 patch.

Best Regards,
Kane
________________________________
寄件者: Peter Maydell <peter.maydell@linaro.org>
寄件日期: 2026年4月30日 下午 07:03
收件者: Kane Chen <kane_chen@aspeedtech.com>
副本: Cédric Le Goater <clg@kaod.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Jamin Lin <jamin_lin@aspeedtech.com>; Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here <qemu-devel@nongnu.org>; Troy Lee <troy_lee@aspeedtech.com>; qemu-stable@nongnu.org <qemu-stable@nongnu.org>; Cédric Le Goater <clg@redhat.com>
主旨: Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

On Tue, 28 Apr 2026 at 06:53, Kane Chen <kane_chen@aspeedtech.com> wrote:
>
> 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 <kane_chen@aspeedtech.com>
> Cc: qemu-stable@nongnu.org
> Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>  hw/misc/aspeed_sbc.c  | 14 +++++++++++---
>  hw/nvram/aspeed_otp.c | 13 ++++++-------
>  2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> index 065e822e70..8b74dca13c 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -159,13 +159,21 @@ 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",
> -                      otp_addr);
> +                      otp_offset);

I would stick to reporting otp_addr here:
 - it's the value the guest actually writes
 - it matches the logging for "invalid OTP addr above"
 - it matches what aspeed_sbc_otp_read() logs in the equivalent case

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[-- Attachment #2: Type: text/html, Size: 6726 bytes --]

Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Cédric Le Goater]

On 4/28/26 07:52, Kane Chen wrote:
> 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 <kane_chen@aspeedtech.com>
> Cc: qemu-stable@nongnu.org
> Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>   hw/misc/aspeed_sbc.c  | 14 +++++++++++---
>   hw/nvram/aspeed_otp.c | 13 ++++++-------
>   2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> index 065e822e70..8b74dca13c 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -159,13 +159,21 @@ 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);

This log event could be on one line.

> +        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",
> -                      otp_addr);
> +                      otp_offset);
>           return false;
>       }
>   
> diff --git a/hw/nvram/aspeed_otp.c b/hw/nvram/aspeed_otp.c
> index a60289000c..1a9d3841b8 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;

I think 'opt_addr' is a bit confusing. It's a word offset.

valid_program_data() could also use an 'otp_offset' arg and
I would introduce a small inline helper to replace variable
'is_odd'

Thanks,

C.



>       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;

Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Peter Maydell]

On Thu, 30 Apr 2026 at 18:26, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 4/28/26 07:52, Kane Chen wrote:
> > 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 <kane_chen@aspeedtech.com>
> > Cc: qemu-stable@nongnu.org
> > Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> >   hw/misc/aspeed_sbc.c  | 14 +++++++++++---
> >   hw/nvram/aspeed_otp.c | 13 ++++++-------
> >   2 files changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> > index 065e822e70..8b74dca13c 100644
> > --- a/hw/misc/aspeed_sbc.c
> > +++ b/hw/misc/aspeed_sbc.c
> > @@ -159,13 +159,21 @@ 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);
>
> This log event could be on one line.
>
> > +        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",
> > -                      otp_addr);
> > +                      otp_offset);
> >           return false;
> >       }
> >
> > diff --git a/hw/nvram/aspeed_otp.c b/hw/nvram/aspeed_otp.c
> > index a60289000c..1a9d3841b8 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;
>
> I think 'opt_addr' is a bit confusing. It's a word offset.

It's the variable name we're currently using for that meaning
everywhere in these two devices, though. It might be better
to keep that cleanup in a separate patch from this one.

-- PMM

Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Cédric Le Goater]

On 4/30/26 19:47, Peter Maydell wrote:
> On Thu, 30 Apr 2026 at 18:26, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 4/28/26 07:52, Kane Chen wrote:
>>> 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 <kane_chen@aspeedtech.com>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
>>> ---
>>>    hw/misc/aspeed_sbc.c  | 14 +++++++++++---
>>>    hw/nvram/aspeed_otp.c | 13 ++++++-------
>>>    2 files changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
>>> index 065e822e70..8b74dca13c 100644
>>> --- a/hw/misc/aspeed_sbc.c
>>> +++ b/hw/misc/aspeed_sbc.c
>>> @@ -159,13 +159,21 @@ 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);
>>
>> This log event could be on one line.
>>
>>> +        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",
>>> -                      otp_addr);
>>> +                      otp_offset);
>>>            return false;
>>>        }
>>>
>>> diff --git a/hw/nvram/aspeed_otp.c b/hw/nvram/aspeed_otp.c
>>> index a60289000c..1a9d3841b8 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;
>>
>> I think 'opt_addr' is a bit confusing. It's a word offset.
> 
> It's the variable name we're currently using for that meaning
> everywhere in these two devices, though. It might be better
> to keep that cleanup in a separate patch from this one.

OK. Let's do that.


C.

Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Cédric Le Goater]

On 4/30/26 13:03, Peter Maydell wrote:
> On Tue, 28 Apr 2026 at 06:53, Kane Chen <kane_chen@aspeedtech.com> wrote:
>>
>> 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 <kane_chen@aspeedtech.com>
>> Cc: qemu-stable@nongnu.org
>> Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
>> ---
>>   hw/misc/aspeed_sbc.c  | 14 +++++++++++---
>>   hw/nvram/aspeed_otp.c | 13 ++++++-------
>>   2 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
>> index 065e822e70..8b74dca13c 100644
>> --- a/hw/misc/aspeed_sbc.c
>> +++ b/hw/misc/aspeed_sbc.c
>> @@ -159,13 +159,21 @@ 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",
>> -                      otp_addr);
>> +                      otp_offset);
> 
> I would stick to reporting otp_addr here:
>   - it's the value the guest actually writes
>   - it matches the logging for "invalid OTP addr above"
>   - it matches what aspeed_sbc_otp_read() logs in the equivalent case
> 
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>



Applied to

     https://github.com/legoater/qemu aspeed-next

with this change.

Peter,

Feel free to pick the change if needed.

Thanks,

C.

RE: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Kane Chen]

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Friday, May 1, 2026 1:57 AM
> To: Peter Maydell <peter.maydell@linaro.org>; Kane Chen
> <kane_chen@aspeedtech.com>
> Cc: Steven Lee <steven_lee@aspeedtech.com>; Troy Lee
> <leetroy@gmail.com>; Jamin Lin <jamin_lin@aspeedtech.com>; Andrew
> Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open
> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>; Troy Lee <troy_lee@aspeedtech.com>;
> qemu-stable@nongnu.org; Cédric Le Goater <clg@redhat.com>
> Subject: Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP
> write operations
> 
> On 4/30/26 13:03, Peter Maydell wrote:
> > On Tue, 28 Apr 2026 at 06:53, Kane Chen <kane_chen@aspeedtech.com>
> wrote:
> >>
> >> 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 <kane_chen@aspeedtech.com>
> >> Cc: qemu-stable@nongnu.org
> >> Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller
> >> model")
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3436
> >> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> >> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> >> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >> ---
> >>   hw/misc/aspeed_sbc.c  | 14 +++++++++++---
> >>   hw/nvram/aspeed_otp.c | 13 ++++++-------
> >>   2 files changed, 17 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> >> 065e822e70..8b74dca13c 100644
> >> --- a/hw/misc/aspeed_sbc.c
> >> +++ b/hw/misc/aspeed_sbc.c
> >> @@ -159,13 +159,21 @@ 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",
> >> -                      otp_addr);
> >> +                      otp_offset);
> >
> > I would stick to reporting otp_addr here:
> >   - it's the value the guest actually writes
> >   - it matches the logging for "invalid OTP addr above"
> >   - it matches what aspeed_sbc_otp_read() logs in the equivalent case
> >
> > Otherwise
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> 
> 
> Applied to
> 
>      https://github.com/legoater/qemu aspeed-next
> 
> with this change.
> 
> Peter,
> 
> Feel free to pick the change if needed.
> 
> Thanks,
> 
> C.

Hi Cédric/Peter,

Thanks for your review and comments.

Currently, the naming of otp_addr and otp_offset is inconsistent, as both are used to represent the user-provided address. For example, when implementing the OTP module, I followed the naming convention used in the write operation (see the link below for reference):
https://github.com/AspeedTech-BMC/zephyr/blob/aspeed-main-v3.7.0/drivers/misc/aspeed/otp_shell.c#L239

However, in the read path, the code uses the term offset instead (see below):
https://github.com/AspeedTech-BMC/zephyr/blob/aspeed-main-v3.7.0/drivers/misc/aspeed/otp_shell.c#L150

To improve consistency, we could align the naming with otp_offset, following the read path convention. However, I'm not certain if this would be more intuitive. Please let me know if you have any further suggestions regarding the otp_addr / otp_offset naming.

Best Regards,
Kane

Re: [PATCH v1 1/1] hw/misc/aspeed_sbc: Add bounds checking for OTP write operations

[Michael Tokarev]

On 28.04.2026 08:52, Kane Chen wrote:
> 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 <kane_chen@aspeedtech.com>
> Cc: qemu-stable@nongnu.org
> Fixes: 1a00754ccf15 ("hw/misc: Add Aspeed Secure Boot Controller model")


FWIW (this change has already landed in the master branch, so it's cast
in stone), I can't find this commit in my git clone of qemu repository.
Neither hash ID nor the name.

Is it this one maybe? https://marc.info/?l=qemu-arm&m=164500409921734,
which is commit e1acf581c9 in qemu?  It has a different commit message
though.

And this patch does not apply to 10.0.x, - in there, there's no
hw/nvram/aspeed_otp.c (688a3dae78 hw/nvram/aspeed_otp: Add ASPEED OTP
memory device model).  I assume it's not needed for 10.0.x, but in
that case, the Fixes: tag isn't even commit e1acf581c9.

Thanks,

/mjt