Hi Philippe,
On Tue, Jun 18, 2024 at 10:34 PM Philippe Mathieu-Daudé <[email protected]>
wrote:
> On 18/6/24 21:11, Zheyu Ma wrote:
> > Thanks for your useful advice!
> >
> > So how about report the issue and return:
>
> We might report the issue to the user, but there should
> be a way the hardware report the issue to the guest software
> running. Usually signaled as error condition, irq, ...
> We need to figure out what check wasn't properly done.
> The spec is the source of truth.
>
Sure.
Although I'm unfamiliar with the device, I'll try to figure it out.
Zheyu
>
> > 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é
> > <[email protected] <mailto:[email protected]>> 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 <[email protected]
> > <mailto:[email protected]>>
> > > ---
> > > 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) {
> >
>
>
