On Tue, May 31, 2022 at 5:20 PM Claudio Fontana <[email protected]> wrote: > > the code in pcibus_get_fw_dev_path contained the potential for a > stack buffer overflow of 1 byte, potentially writing to the stack an > extra NUL byte. > > This overflow could happen if the PCI slot is >= 0x10000000, > and the PCI function is >= 0x10000000, due to the size parameter > of snprintf being incorrectly calculated in the call: > > if (PCI_FUNC(d->devfn)) > snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn)); > > since the off obtained from a previous call to snprintf is added > instead of subtracted from the total available size of the buffer. > > Without the accurate size guard from snprintf, we end up writing in the > worst case: > > name (32) + "@" (1) + SLOT (8) + "," (1) + FUNC (8) + term NUL (1) = 51 bytes > > In order to provide something more robust, replace all of the code in > pcibus_get_fw_dev_path with a single call to g_strdup_printf, > so there is no need to rely on manual calculations. > > Found by compiling QEMU with FORTIFY_SOURCE=3 as the error: > > *** buffer overflow detected ***: terminated > > Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted. > [Switching to Thread 0x7ffff642c380 (LWP 121307)] > 0x00007ffff71ff55c in __pthread_kill_implementation () from /lib64/libc.so.6 > (gdb) bt > #0 0x00007ffff71ff55c in __pthread_kill_implementation () at > /lib64/libc.so.6 > #1 0x00007ffff71ac6f6 in raise () at /lib64/libc.so.6 > #2 0x00007ffff7195814 in abort () at /lib64/libc.so.6 > #3 0x00007ffff71f279e in __libc_message () at /lib64/libc.so.6 > #4 0x00007ffff729767a in __fortify_fail () at /lib64/libc.so.6 > #5 0x00007ffff7295c36 in () at /lib64/libc.so.6 > #6 0x00007ffff72957f5 in __snprintf_chk () at /lib64/libc.so.6 > #7 0x0000555555b1c1fd in pcibus_get_fw_dev_path () > #8 0x0000555555f2bde4 in qdev_get_fw_dev_path_helper.constprop () > #9 0x0000555555f2bd86 in qdev_get_fw_dev_path_helper.constprop () > #10 0x00005555559a6e5d in get_boot_device_path () > #11 0x00005555559a712c in get_boot_devices_list () > #12 0x0000555555b1a3d0 in fw_cfg_machine_reset () > #13 0x0000555555bf4c2d in pc_machine_reset () > #14 0x0000555555c66988 in qemu_system_reset () > #15 0x0000555555a6dff6 in qdev_machine_creation_done () > #16 0x0000555555c79186 in qmp_x_exit_preconfig.part () > #17 0x0000555555c7b459 in qemu_init () > #18 0x0000555555960a29 in main () > > Found-by: Dario Faggioli <Dario Faggioli <[email protected]> > Found-by: Martin Liška <[email protected]>
I think Reported-by: is preferred. > Cc: [email protected] > Signed-off-by: Claudio Fontana <[email protected]> > --- > hw/pci/pci.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index a9b37f8000..6e7015329c 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2640,15 +2640,15 @@ static char *pci_dev_fw_name(DeviceState *dev, char > *buf, int len) > static char *pcibus_get_fw_dev_path(DeviceState *dev) > { > PCIDevice *d = (PCIDevice *)dev; > - char path[50], name[33]; > - int off; > - > - off = snprintf(path, sizeof(path), "%s@%x", > - pci_dev_fw_name(dev, name, sizeof name), > - PCI_SLOT(d->devfn)); > - if (PCI_FUNC(d->devfn)) > - snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn)); > - return g_strdup(path); > + char name[33]; > + int has_func = !!PCI_FUNC(d->devfn); > + > + return g_strdup_printf("%s@%x%s%.*x", I was experimenting with printf dynamic precision field with hex and it was not quite working as expected. In particular, with precision 0, I was still able to print a single hex digit. That is the following still outputs 5 in stderr : > fprintf(stderr, "%.*x\n", 0, 5); > + pci_dev_fw_name(dev, name, sizeof(name)), > + PCI_SLOT(d->devfn), > + has_func ? "," : "", > + has_func, > + PCI_FUNC(d->devfn)); > } > > static char *pcibus_get_dev_path(DeviceState *dev) > -- > 2.26.2 > >
