Re: QEMU | READ memory access in /hw/acpi/pcihp.c (#770)

[Igor Mammedov]

On Mon, 13 Dec 2021 05:33:43 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 13, 2021 at 08:43:55AM +0100, Thomas Huth wrote:
> > 
> >  Hi Michael, hi Igor,
> > 
> > just FYI, a crash has been reported in the ACPI code ...
> > by the way, do you have a gitlab account, so you could be put on CC: for
> > such bugs there, too?
> > 
> >  Regards,
> >   Thomas
> > 
> > 
> > -------- Forwarded Message --------
> > Subject:    QEMU | READ memory access in /hw/acpi/pcihp.c (#770)
> > Date:       Sun, 12 Dec 2021 15:03:50 +0000
> > From:       LucasLeeNDS (@LucasLeeNDS) <gitlab@mg.gitlab.com>
> > Reply-To:   QEMU / QEMU
> > <incoming+b01bab3884ecadb087cf49724e7b33aa@incoming.gitlab.com>
> > To:         thuth@redhat.com
> > 
> > 
> > 
> > GitLab
> > 
> > LucasLeeNDS <https://gitlab.com/LucasLeeNDS> created an issue: #770
> > <https://gitlab.com/qemu-project/qemu/-/issues/770>
> > 
> > Hello qemu team, An invalid pointer initialization issue was found in
> > /hw/acpi/pcihp.c:470:9 of QEMU in versions 6.2.0-rc2.
> > 
> > *Reproducer*
> > 
> > |cat << EOF | ./qemu-system-i386 \ -M pc -nodefaults -netdev user,id=user0
> > -device virtio-net,netdev=user0 \ -qtest stdio outl 0xcf8 0x80000b00 inw
> > 0xcfc outl 0xcf8 0x80000b04 inw 0xcfc outl 0xcf8 0x80000b04 outw 0xcfc 0x7
> > outl 0xcf8 0x80000b04 inw 0xcfc outl 0xcf8 0x80000000 inw 0xcfc outl 0xcf8
> > 0x80000004 inw 0xcfc outl 0xcf8 0x80000004 outw 0xcfc 0x7 outl 0xcf8
> > 0x80000004 inw 0xcfc outl 0xcf8 0x80000800 inw 0xcfc outl 0xcf8 0x80000804
> > inw 0xcfc outl 0xcf8 0x80000804 outw 0xcfc 0x7 outl 0xcf8 0x80000804 inw
> > 0xcfc outl 0xcf8 0x80000900 inw 0xcfc outl 0xcf8 0x80000920 outl 0xcfc
> > 0xffffffff outl 0xcf8 0x80000920 inl 0xcfc outl 0xcf8 0x80000920 outl 0xcfc
> > 0xc001 outl 0xcf8 0x80000904 inw 0xcfc outl 0xcf8 0x80000904 outw 0xcfc 0x7
> > outl 0xcf8 0x80000904 inw 0xcfc outl 0xcf8 0x80001000 inw 0xcfc outl 0xcf8
> > 0x80001010 outl 0xcfc 0xffffffff outl 0xcf8 0x80001010 inl 0xcfc outl 0xcf8
> > 0x80001010 outl 0xcfc 0xc021 outl 0xcf8 0x80001014 outl 0xcfc 0xffffffff
> > outl 0xcf8 0x80001014 inl 0xcfc outl 0xcf8 0x80001014 outl 0xcfc 0xe0000000
> > outl 0xcf8 0x80001020 outl 0xcfc 0xffffffff outl 0xcf8 0x80001020 inl 0xcfc
> > outl 0xcf8 0x80001020 outl 0xcfc 0xe0004000 outl 0xcf8 0x80001004 inw 0xcfc
> > outl 0xcf8 0x80001004 outw 0xcfc 0x7 outl 0xcf8 0x80001004 inw 0xcfc
> > clock_step outl 0xae10 0x15 outl 0xae10 0x585a5564 outl 0xae10 0x15 outl
> > 0xcf8 0x80000b06 outl 0xcfc 0xdd58fb5a outl 0xae14 0x64296572 clock_step
> > outl 0xae10 0x15 outl 0xae10 0x585a5564 outl 0xae10 0x15 outl 0xcf8
> > 0x80000b06 outl 0xcfc 0xdd58fb5a outl 0xae14 0x64296572 EOF|
> > 
> > *Stack-Trace*
> > 
> > |AddressSanitizer:DEADLYSIGNAL
> > =================================================================
> > ==4191==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000050 (pc
> > 0x564df8697958 bp 0x7ffe620c13f0 sp 0x7ffe620c12a0 T0) ==4191==The signal is
> > caused by a READ memory access. ==4191==Hint: address points to the zero
> > page. #0 0x564df8697958 in pci_write
> > /home/test/Desktop/qemu-6.2.0-rc2/build/../hw/acpi/pcihp.c:470:9 #1
> > 0x564df941eb3c in memory_region_write_accessor  
> 
> So it's this line:
> 
>         QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
> 
> in pci_write
> 
> probably because we supplied an invalid selector so bus is NULL?
> 
> Just checking that should do the job I think ...
> Igor, what do you think?

It should fix the issue, would you like to send a formal patch?

(as for impact, it should not case issue in normal use-case,
max possible harm would be crashing qemu/VM if user has write
rights to pcihp IO window (typically restricted to superuser),
that's what test case/reproducer was doing)

> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 30405b5113..a5e182dd3a 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -491,6 +491,9 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t 
> data,
>          }
>  
>          bus = acpi_pcihp_find_hotplug_bus(s, s->hotplug_select);
> +        if (!bus) {
> +            break;
> +        }
>          QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
>              Object *o = OBJECT(kid->child);
>              PCIDevice *dev = PCI_DEVICE(o);