Re: [PATCH] pci: fix overflow in snprintf string formatting

[Michael S. Tsirkin]

On Tue, May 31, 2022 at 01:47:07PM +0200, Claudio Fontana 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 <dfaggioli@suse.com>
> Found-by: Martin Liška <martin.liska@suse.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Claudio Fontana <cfontana@suse.de>

Queued, thanks!

> ---
>  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",
> +                           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