Re: [PATCH] block: m25p80: Fix heap-buffer-overflow in flash_erase function

[Zheyu Ma <zheyuma97@gmail.com>]

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

Thanks for your useful advice!

So how about report the issue and return:

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8dec134832..2121b43708 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -617,6 +617,12 @@ static void flash_erase(Flash *s, int offset, FlashCMD
cmd)
         abort();
     }

+    if (offset + len > s->size) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "M25P80: Erase operation exceeds storage size\n");
+        return;
+    }
+
     trace_m25p80_flash_erase(s, offset, len);

     if ((s->pi->flags & capa_to_assert) != capa_to_assert) {

regards,
Zheyu

On Tue, Jun 18, 2024 at 5:35 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Hi Zheyu,
>
> On 18/6/24 17:23, Zheyu Ma wrote:
> > This patch fixes a heap-buffer-overflow issue in the flash_erase function
> > of the m25p80 flash memory emulation. The overflow occurs when the
> > combination of offset and length exceeds the allocated memory for the
> > storage. The patch adds a check to ensure that the erase length does not
> > exceed the storage size and adjusts the length accordingly if necessary.
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display none \
> > -machine accel=qtest, -m 512M -machine kudo-bmc -qtest stdio
> > writeq 0xc0000010 0x6
> > writel 0xc000000c 0x9
> > writeq 0xc0000010 0xf27f9412
> > writeq 0xc000000f 0x2b5cdc26
> > writeq 0xc000000c 0xffffffffffffffff
> > writeq 0xc000000c 0xffffffffffffffff
> > writeq 0xc000000c 0xffffffffffffffff
> > writel 0xc000000c 0x9
> > writeq 0xc000000c 0x9
> > EOF
> >
> > ASan log:
> > ==2614308==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x7fd3fb7fc000 at pc 0x55aa77a442dc bp 0x7fffaa155900 sp 0x7fffaa1550c8
> > WRITE of size 65536 at 0x7fd3fb7fc000 thread T0
> >      #0 0x55aa77a442db in __asan_memset
> llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3
> >      #1 0x55aa77e7e6b3 in flash_erase hw/block/m25p80.c:631:5
> >      #2 0x55aa77e6f8b1 in complete_collecting_data
> hw/block/m25p80.c:773:9
> >      #3 0x55aa77e6aaa9 in m25p80_transfer8 hw/block/m25p80.c:1550:13
> >      #4 0x55aa78e9a691 in ssi_transfer_raw_default hw/ssi/ssi.c:92:16
> >      #5 0x55aa78e996c0 in ssi_transfer hw/ssi/ssi.c:165:14
> >      #6 0x55aa78e8d76a in npcm7xx_fiu_uma_transaction
> hw/ssi/npcm7xx_fiu.c:336:9
> >      #7 0x55aa78e8be4b in npcm7xx_fiu_ctrl_write
> hw/ssi/npcm7xx_fiu.c:428:13
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >   hw/block/m25p80.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 8dec134832..e9a59f6616 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -617,6 +617,12 @@ static void flash_erase(Flash *s, int offset,
> FlashCMD cmd)
> >           abort();
> >       }
> >
> > +    if (offset + len > s->size) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "M25P80: Erase exceeds storage size, adjusting
> length\n");
>
> Usually hardware doesn't "adjust" but report error earlier.
>
> > +        len = s->size - offset;
> > +    }
> > +
> >       trace_m25p80_flash_erase(s, offset, len);
> >
> >       if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
>
>

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