[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] pci: fix overflow in snprintf string formatting
From: |
Claudio Fontana |
Subject: |
Re: [PATCH] pci: fix overflow in snprintf string formatting |
Date: |
Tue, 31 May 2022 14:45:04 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 |
On 5/31/22 14:26, Ani Sinha wrote:
> On Tue, May 31, 2022 at 5:20 PM Claudio Fontana <cfontana@suse.de> 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>
>
> I think Reported-by: is preferred.
>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>> 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);
Hi Ani, both the precision and the value need to be zero to omit the value,
and this is what the patch relies on.
Ciao,
Claudio
>
>
>> + 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
>>
>>