* [PATCH RESEND 0/3] fix some warnings by static code scan tool @ 2020-02-24 6:42 Longpeng(Mike) 2020-02-24 6:42 ` [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read Longpeng(Mike) ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Longpeng(Mike) @ 2020-02-24 6:42 UTC (permalink / raw) To: mst, alex.williamson Cc: longpeng2, arei.gonglei, huangzhichao, qemu-devel, weifuqiang From: Longpeng <longpeng2@huawei.com> Hi guys, Our tool find some potential issues in QEMU source code, maybe they're misreported, hope you could have a look at them. Longpeng (Mike) (3): vfio/pci: fix a null pointer reference in vfio_rom_read vhost: fix a null pointer reference of vhost_log util/pty: fix a null pointer reference in qemu_openpty_raw hw/vfio/pci.c | 13 ++++++++----- hw/virtio/vhost.c | 19 +++++++++++++++++-- util/qemu-openpty.c | 10 ++-------- 3 files changed, 27 insertions(+), 15 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-02-24 6:42 [PATCH RESEND 0/3] fix some warnings by static code scan tool Longpeng(Mike) @ 2020-02-24 6:42 ` Longpeng(Mike) 2020-02-24 16:04 ` Alex Williamson 2020-02-24 6:42 ` [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log Longpeng(Mike) 2020-02-24 6:42 ` [PATCH RESEND 3/3] util/pty: fix a null pointer reference in qemu_openpty_raw Longpeng(Mike) 2 siblings, 1 reply; 23+ messages in thread From: Longpeng(Mike) @ 2020-02-24 6:42 UTC (permalink / raw) To: mst, alex.williamson Cc: longpeng2, arei.gonglei, huangzhichao, qemu-devel, weifuqiang From: Longpeng <longpeng2@huawei.com> vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in some situation (though I've not encountered yet), maybe we should avoid the VM abort. Signed-off-by: Longpeng <longpeng2@huawei.com> --- hw/vfio/pci.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 5e75a95..ed798ae 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) } } -static void vfio_pci_load_rom(VFIOPCIDevice *vdev) +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev) { struct vfio_region_info *reg_info; uint64_t size; @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) if (vfio_get_region_info(&vdev->vbasedev, VFIO_PCI_ROM_REGION_INDEX, ®_info)) { error_report("vfio: Error getting ROM info: %m"); - return; + return false; } trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned long)reg_info->size, @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) error_printf("Device option ROM contents are probably invalid " "(check dmesg).\nSkip option ROM probe with rombar=0, " "or load from file with romfile=\n"); - return; + return false; } vdev->rom = g_malloc(size); @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) data[6] = -csum; } } + + return true; } static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) uint64_t data = 0; /* Load the ROM lazily when the guest tries to read it */ - if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { - vfio_pci_load_rom(vdev); + if (unlikely(!vdev->rom && !vdev->rom_read_failed) && + !vfio_pci_load_rom(vdev)) { + return 0; } memcpy(&val, vdev->rom + addr, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-02-24 6:42 ` [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read Longpeng(Mike) @ 2020-02-24 16:04 ` Alex Williamson 2020-02-24 23:48 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-11 7:04 ` Markus Armbruster 0 siblings, 2 replies; 23+ messages in thread From: Alex Williamson @ 2020-02-24 16:04 UTC (permalink / raw) To: Longpeng(Mike); +Cc: weifuqiang, arei.gonglei, huangzhichao, qemu-devel, mst On Mon, 24 Feb 2020 14:42:17 +0800 "Longpeng(Mike)" <longpeng2@huawei.com> wrote: > From: Longpeng <longpeng2@huawei.com> > > vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in > some situation (though I've not encountered yet), maybe we should > avoid the VM abort. > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/vfio/pci.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 5e75a95..ed798ae 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) > } > } > > -static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev) > { > struct vfio_region_info *reg_info; > uint64_t size; > @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > if (vfio_get_region_info(&vdev->vbasedev, > VFIO_PCI_ROM_REGION_INDEX, ®_info)) { > error_report("vfio: Error getting ROM info: %m"); > - return; > + return false; > } > > trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned long)reg_info->size, > @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > error_printf("Device option ROM contents are probably invalid " > "(check dmesg).\nSkip option ROM probe with rombar=0, " > "or load from file with romfile=\n"); > - return; > + return false; > } > > vdev->rom = g_malloc(size); > @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > data[6] = -csum; > } > } > + > + return true; > } > > static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > uint64_t data = 0; > > /* Load the ROM lazily when the guest tries to read it */ > - if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { > - vfio_pci_load_rom(vdev); > + if (unlikely(!vdev->rom && !vdev->rom_read_failed) && > + !vfio_pci_load_rom(vdev)) { > + return 0; > } > > memcpy(&val, vdev->rom + addr, Looks like an obvious bug, until you look at the rest of this memcpy(): memcpy(&val, vdev->rom + addr, (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); IOW, we'll do a zero sized memcpy() if rom_size is zero, so there's no risk of the concern identified in the commit log. This patch is unnecessary. Thanks, Alex ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-02-24 16:04 ` Alex Williamson @ 2020-02-24 23:48 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-10 16:11 ` Alex Williamson 2020-03-11 7:04 ` Markus Armbruster 1 sibling, 1 reply; 23+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-02-24 23:48 UTC (permalink / raw) To: Alex Williamson; +Cc: weifuqiang, arei.gonglei, huangzhichao, qemu-devel, mst On 2020/2/25 0:04, Alex Williamson wrote: > On Mon, 24 Feb 2020 14:42:17 +0800 > "Longpeng(Mike)" <longpeng2@huawei.com> wrote: > >> From: Longpeng <longpeng2@huawei.com> >> >> vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in >> some situation (though I've not encountered yet), maybe we should >> avoid the VM abort. >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> >> --- >> hw/vfio/pci.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 5e75a95..ed798ae 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) >> } >> } >> >> -static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev) >> { >> struct vfio_region_info *reg_info; >> uint64_t size; >> @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> if (vfio_get_region_info(&vdev->vbasedev, >> VFIO_PCI_ROM_REGION_INDEX, ®_info)) { >> error_report("vfio: Error getting ROM info: %m"); >> - return; >> + return false; >> } >> >> trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned long)reg_info->size, >> @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> error_printf("Device option ROM contents are probably invalid " >> "(check dmesg).\nSkip option ROM probe with rombar=0, " >> "or load from file with romfile=\n"); >> - return; >> + return false; >> } >> >> vdev->rom = g_malloc(size); >> @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> data[6] = -csum; >> } >> } >> + >> + return true; >> } >> >> static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) >> @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) >> uint64_t data = 0; >> >> /* Load the ROM lazily when the guest tries to read it */ >> - if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { >> - vfio_pci_load_rom(vdev); >> + if (unlikely(!vdev->rom && !vdev->rom_read_failed) && >> + !vfio_pci_load_rom(vdev)) { >> + return 0; >> } >> >> memcpy(&val, vdev->rom + addr, > > Looks like an obvious bug, until you look at the rest of this memcpy(): > > memcpy(&val, vdev->rom + addr, > (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); > > IOW, we'll do a zero sized memcpy() if rom_size is zero, so there's no > risk of the concern identified in the commit log. This patch is > unnecessary. Thanks, > Oh, I missed that, sorry for make the noise, thanks > Alex > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-02-24 23:48 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-10 16:11 ` Alex Williamson 2020-03-10 23:14 ` Laszlo Ersek 0 siblings, 1 reply; 23+ messages in thread From: Alex Williamson @ 2020-03-10 16:11 UTC (permalink / raw) To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) Cc: weifuqiang, mst, qemu-devel, arei.gonglei, huangzhichao, Laszlo Ersek On Tue, 25 Feb 2020 07:48:33 +0800 "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)" <longpeng2@huawei.com> wrote: > On 2020/2/25 0:04, Alex Williamson wrote: > > On Mon, 24 Feb 2020 14:42:17 +0800 > > "Longpeng(Mike)" <longpeng2@huawei.com> wrote: > > > >> From: Longpeng <longpeng2@huawei.com> > >> > >> vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in > >> some situation (though I've not encountered yet), maybe we should > >> avoid the VM abort. > >> > >> Signed-off-by: Longpeng <longpeng2@huawei.com> > >> --- > >> hw/vfio/pci.c | 13 ++++++++----- > >> 1 file changed, 8 insertions(+), 5 deletions(-) > >> > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > >> index 5e75a95..ed798ae 100644 > >> --- a/hw/vfio/pci.c > >> +++ b/hw/vfio/pci.c > >> @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) > >> } > >> } > >> > >> -static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> { > >> struct vfio_region_info *reg_info; > >> uint64_t size; > >> @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> if (vfio_get_region_info(&vdev->vbasedev, > >> VFIO_PCI_ROM_REGION_INDEX, ®_info)) { > >> error_report("vfio: Error getting ROM info: %m"); > >> - return; > >> + return false; > >> } > >> > >> trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned long)reg_info->size, > >> @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> error_printf("Device option ROM contents are probably invalid " > >> "(check dmesg).\nSkip option ROM probe with rombar=0, " > >> "or load from file with romfile=\n"); > >> - return; > >> + return false; > >> } > >> > >> vdev->rom = g_malloc(size); > >> @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> data[6] = -csum; > >> } > >> } > >> + > >> + return true; > >> } > >> > >> static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > >> @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > >> uint64_t data = 0; > >> > >> /* Load the ROM lazily when the guest tries to read it */ > >> - if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { > >> - vfio_pci_load_rom(vdev); > >> + if (unlikely(!vdev->rom && !vdev->rom_read_failed) && > >> + !vfio_pci_load_rom(vdev)) { > >> + return 0; > >> } > >> > >> memcpy(&val, vdev->rom + addr, > > > > Looks like an obvious bug, until you look at the rest of this memcpy(): > > > > memcpy(&val, vdev->rom + addr, > > (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); > > > > IOW, we'll do a zero sized memcpy() if rom_size is zero, so there's no > > risk of the concern identified in the commit log. This patch is > > unnecessary. Thanks, > > > Oh, I missed that, sorry for make the noise, thanks Actually, not noise. After some internal discussion thanks to Laszlo, it seems that while memcpy() with a zero size seems to do the right thing, the behavior for any case where we pass a null pointer is not actually defined. However, there's still a bug in the implementation of the fix above, if vdev->rom_read_failed is set, we'll still fall through to the memcpy. I think there's also another bug in the current implementation that we initialize data to zero but we'll overwrite it with the uninitialized 'val' in the switch statement. I think the below resolves both. I'll formally post it after a bit of testing: commit 2088fc1e1f426b98e9ca4d7bcdbe53d886a18c37 Author: Alex Williamson <alex.williamson@redhat.com> Date: Tue Mar 10 10:04:36 2020 -0600 vfio/pci: Use defined memcpy() behavior vfio_rom_read() relies on memcpy() doing the logically correct thing, ie. safely copying zero bytes from a NULL pointer when rom_size is zero, rather than the spec definition, which is undefined when the source or target pointers are NULL. Resolve this by wrapping the call in the condition expressed previously by the ternary. Additionally, we still use @val to fill data based on the provided @size regardless of mempcy(), so we should initialize @val rather than @data. Reported-by: Longpeng <longpeng2@huawei.com> Reported-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 5e75a95129ac..b0799cdc28ad 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -859,16 +859,17 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) uint16_t word; uint32_t dword; uint64_t qword; - } val; - uint64_t data = 0; + } val = { 0 }; + uint64_t data; /* Load the ROM lazily when the guest tries to read it */ if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { vfio_pci_load_rom(vdev); } - memcpy(&val, vdev->rom + addr, - (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); + if (addr < vdev->rom_size) { + memcpy(&val, vdev->rom + addr, MIN(size, vdev->rom_size - addr)); + } switch (size) { case 1: ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-10 16:11 ` Alex Williamson @ 2020-03-10 23:14 ` Laszlo Ersek 2020-03-11 1:36 ` Alex Williamson 0 siblings, 1 reply; 23+ messages in thread From: Laszlo Ersek @ 2020-03-10 23:14 UTC (permalink / raw) To: Alex Williamson, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) Cc: weifuqiang, mst, Markus Armbruster, qemu-devel, arei.gonglei, huangzhichao On 03/10/20 17:11, Alex Williamson wrote: > commit 2088fc1e1f426b98e9ca4d7bcdbe53d886a18c37 > Author: Alex Williamson <alex.williamson@redhat.com> > Date: Tue Mar 10 10:04:36 2020 -0600 > > vfio/pci: Use defined memcpy() behavior > > vfio_rom_read() relies on memcpy() doing the logically correct thing, > ie. safely copying zero bytes from a NULL pointer when rom_size is > zero, rather than the spec definition, which is undefined when the > source or target pointers are NULL. Resolve this by wrapping the > call in the condition expressed previously by the ternary. > > Additionally, we still use @val to fill data based on the provided > @size regardless of mempcy(), so we should initialize @val rather > than @data. > > Reported-by: Longpeng <longpeng2@huawei.com> > Reported-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 5e75a95129ac..b0799cdc28ad 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -859,16 +859,17 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > uint16_t word; > uint32_t dword; > uint64_t qword; > - } val; > - uint64_t data = 0; > + } val = { 0 }; > + uint64_t data; > > /* Load the ROM lazily when the guest tries to read it */ > if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { > vfio_pci_load_rom(vdev); > } > > - memcpy(&val, vdev->rom + addr, > - (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); > + if (addr < vdev->rom_size) { > + memcpy(&val, vdev->rom + addr, MIN(size, vdev->rom_size - addr)); > + } > > switch (size) { > case 1: Regarding the pre-patch code: My understanding is that the memcpy() could be reached with a guest-originated "addr" even if "vdev->rom" was NULL. If that's the case, then the pre-patch code invokes undefined behavior regardless of memcpy(), because it performs pointer arithmetic on a null pointer (not to mention that the type of that pointer is (void *)....) Regarding the proposed change: (addr < vdev->rom_size) requires that "vdev->rom_size" be positive. In that case, I assume that - "vdev->rom" is not NULL, and - MIN(size, vdev->rom_size - addr) bytes are "in range" for the object allocated at "vdev->rom". So from a memcpy() and range perspective, the patch looks OK. But there's still a wart I dislike: we should never perform pointer arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or (unsigned char*) first. Here's an excerpt from the ISO C99 standard: -v- 6.5.6 Additive operators Constraints 2 For addition, either both operands shall have arithmetic type, or one operand shall be a pointer to an object type and the other shall have integer type. [...] -^- A "pointer-to-void" is not a "pointer to an object type", because "void" is not an object type -- it is an incomplete type that cannot be completed: -v- 6.2.5 Types 1 [...] Types are partitioned into object types (types that fully describe objects), function types (types that describe functions), and incomplete types (types that describe objects but lack information needed to determine their sizes). [...] 19 The void type comprises an empty set of values; it is an incomplete type that cannot be completed. -^- For a different illustration, (vdev->rom + addr) is equivalent to &(vdev->rom[addr]) -- and we clearly can't have an "array of void". This anti-pattern (of doing pointer arithmetic on (void*)) likely comes from a guarantee that the standard does make, in the same "6.2.5 Types" section: -v- 27 A pointer to void shall have the same representation and alignment requirements as a pointer to a character type. 39) [...] Footnote 39: The same representation and alignment requirements are meant to imply interchangeability as arguments to functions, return values from functions, and members of unions. -^- It does not extend to the "+" operator. Thanks Laszlo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-10 23:14 ` Laszlo Ersek @ 2020-03-11 1:36 ` Alex Williamson 2020-03-11 7:08 ` Markus Armbruster 2020-03-11 10:26 ` Laszlo Ersek 0 siblings, 2 replies; 23+ messages in thread From: Alex Williamson @ 2020-03-11 1:36 UTC (permalink / raw) To: Laszlo Ersek Cc: weifuqiang, mst, qemu-devel, Markus Armbruster, arei.gonglei, huangzhichao, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) On Wed, 11 Mar 2020 00:14:31 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 03/10/20 17:11, Alex Williamson wrote: > > > commit 2088fc1e1f426b98e9ca4d7bcdbe53d886a18c37 > > Author: Alex Williamson <alex.williamson@redhat.com> > > Date: Tue Mar 10 10:04:36 2020 -0600 > > > > vfio/pci: Use defined memcpy() behavior > > > > vfio_rom_read() relies on memcpy() doing the logically correct thing, > > ie. safely copying zero bytes from a NULL pointer when rom_size is > > zero, rather than the spec definition, which is undefined when the > > source or target pointers are NULL. Resolve this by wrapping the > > call in the condition expressed previously by the ternary. > > > > Additionally, we still use @val to fill data based on the provided > > @size regardless of mempcy(), so we should initialize @val rather > > than @data. > > > > Reported-by: Longpeng <longpeng2@huawei.com> > > Reported-by: Laszlo Ersek <lersek@redhat.com> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index 5e75a95129ac..b0799cdc28ad 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -859,16 +859,17 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > > uint16_t word; > > uint32_t dword; > > uint64_t qword; > > - } val; > > - uint64_t data = 0; > > + } val = { 0 }; > > + uint64_t data; > > > > /* Load the ROM lazily when the guest tries to read it */ > > if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { > > vfio_pci_load_rom(vdev); > > } > > > > - memcpy(&val, vdev->rom + addr, > > - (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); > > + if (addr < vdev->rom_size) { > > + memcpy(&val, vdev->rom + addr, MIN(size, vdev->rom_size - addr)); > > + } > > > > switch (size) { > > case 1: > > Regarding the pre-patch code: > > My understanding is that the memcpy() could be reached with a > guest-originated "addr" even if "vdev->rom" was NULL. If that's the > case, then the pre-patch code invokes undefined behavior regardless of > memcpy(), because it performs pointer arithmetic on a null pointer (not > to mention that the type of that pointer is (void *)....) > > Regarding the proposed change: > > (addr < vdev->rom_size) requires that "vdev->rom_size" be positive. In > that case, I assume that > > - "vdev->rom" is not NULL, and > - MIN(size, vdev->rom_size - addr) bytes are "in range" for the object > allocated at "vdev->rom". > > So from a memcpy() and range perspective, the patch looks OK. But > there's still a wart I dislike: we should never perform pointer > arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or > (unsigned char*) first. > > Here's an excerpt from the ISO C99 standard: > > -v- > 6.5.6 Additive operators > > Constraints > > 2 For addition, either both operands shall have arithmetic type, or one > operand shall be a pointer to an object type and the other shall have > integer type. [...] > -^- > > A "pointer-to-void" is not a "pointer to an object type", because "void" > is not an object type -- it is an incomplete type that cannot be completed: > > -v- > 6.2.5 Types > > 1 [...] Types are partitioned into object types (types that fully > describe objects), function types (types that describe functions), and > incomplete types (types that describe objects but lack information > needed to determine their sizes). > > [...] > > 19 The void type comprises an empty set of values; it is an incomplete > type that cannot be completed. > -^- > > For a different illustration, (vdev->rom + addr) is equivalent to > &(vdev->rom[addr]) -- and we clearly can't have an "array of void". > > This anti-pattern (of doing pointer arithmetic on (void*)) likely comes > from a guarantee that the standard does make, in the same "6.2.5 Types" > section: > > -v- > 27 A pointer to void shall have the same representation and alignment > requirements as a pointer to a character type. 39) [...] > > Footnote 39: The same representation and alignment requirements are > meant to imply interchangeability as arguments to > functions, return values from functions, and members of > unions. > -^- > > It does not extend to the "+" operator. GNU C specifically allows arithmetic on pointers and defines the size of a void as 1. I'll comply, but this makes me want to stab myself in the face :-\ Thanks, Alex ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-11 1:36 ` Alex Williamson @ 2020-03-11 7:08 ` Markus Armbruster 2020-03-11 10:28 ` Laszlo Ersek 2020-03-11 10:26 ` Laszlo Ersek 1 sibling, 1 reply; 23+ messages in thread From: Markus Armbruster @ 2020-03-11 7:08 UTC (permalink / raw) To: Alex Williamson Cc: mst, weifuqiang, qemu-devel, arei.gonglei, huangzhichao, Longpeng (Mike, Cloud Infrastructure Service Product Dept.), Laszlo Ersek Alex Williamson <alex.williamson@redhat.com> writes: > On Wed, 11 Mar 2020 00:14:31 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: [...] >> So from a memcpy() and range perspective, the patch looks OK. But >> there's still a wart I dislike: we should never perform pointer >> arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or >> (unsigned char*) first. >> >> Here's an excerpt from the ISO C99 standard: >> >> -v- >> 6.5.6 Additive operators >> >> Constraints >> >> 2 For addition, either both operands shall have arithmetic type, or one >> operand shall be a pointer to an object type and the other shall have >> integer type. [...] >> -^- >> >> A "pointer-to-void" is not a "pointer to an object type", because "void" >> is not an object type -- it is an incomplete type that cannot be completed: >> >> -v- >> 6.2.5 Types >> >> 1 [...] Types are partitioned into object types (types that fully >> describe objects), function types (types that describe functions), and >> incomplete types (types that describe objects but lack information >> needed to determine their sizes). >> >> [...] >> >> 19 The void type comprises an empty set of values; it is an incomplete >> type that cannot be completed. >> -^- >> >> For a different illustration, (vdev->rom + addr) is equivalent to >> &(vdev->rom[addr]) -- and we clearly can't have an "array of void". >> >> This anti-pattern (of doing pointer arithmetic on (void*)) likely comes >> from a guarantee that the standard does make, in the same "6.2.5 Types" >> section: >> >> -v- >> 27 A pointer to void shall have the same representation and alignment >> requirements as a pointer to a character type. 39) [...] >> >> Footnote 39: The same representation and alignment requirements are >> meant to imply interchangeability as arguments to >> functions, return values from functions, and members of >> unions. >> -^- >> >> It does not extend to the "+" operator. > > GNU C specifically allows arithmetic on pointers and defines the size > of a void as 1. I'll comply, but this makes me want to stab myself in > the face :-\ Thanks, We rely on GNU C extensions all over theplace. Making the code uglier to avoid relying on this one here makes no sense to me. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-11 7:08 ` Markus Armbruster @ 2020-03-11 10:28 ` Laszlo Ersek 0 siblings, 0 replies; 23+ messages in thread From: Laszlo Ersek @ 2020-03-11 10:28 UTC (permalink / raw) To: Markus Armbruster, Alex Williamson Cc: mst, weifuqiang, qemu-devel, arei.gonglei, huangzhichao, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) On 03/11/20 08:08, Markus Armbruster wrote: > Alex Williamson <alex.williamson@redhat.com> writes: > >> On Wed, 11 Mar 2020 00:14:31 +0100 >> Laszlo Ersek <lersek@redhat.com> wrote: > [...] >>> So from a memcpy() and range perspective, the patch looks OK. But >>> there's still a wart I dislike: we should never perform pointer >>> arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or >>> (unsigned char*) first. >>> >>> Here's an excerpt from the ISO C99 standard: >>> >>> -v- >>> 6.5.6 Additive operators >>> >>> Constraints >>> >>> 2 For addition, either both operands shall have arithmetic type, or one >>> operand shall be a pointer to an object type and the other shall have >>> integer type. [...] >>> -^- >>> >>> A "pointer-to-void" is not a "pointer to an object type", because "void" >>> is not an object type -- it is an incomplete type that cannot be completed: >>> >>> -v- >>> 6.2.5 Types >>> >>> 1 [...] Types are partitioned into object types (types that fully >>> describe objects), function types (types that describe functions), and >>> incomplete types (types that describe objects but lack information >>> needed to determine their sizes). >>> >>> [...] >>> >>> 19 The void type comprises an empty set of values; it is an incomplete >>> type that cannot be completed. >>> -^- >>> >>> For a different illustration, (vdev->rom + addr) is equivalent to >>> &(vdev->rom[addr]) -- and we clearly can't have an "array of void". >>> >>> This anti-pattern (of doing pointer arithmetic on (void*)) likely comes >>> from a guarantee that the standard does make, in the same "6.2.5 Types" >>> section: >>> >>> -v- >>> 27 A pointer to void shall have the same representation and alignment >>> requirements as a pointer to a character type. 39) [...] >>> >>> Footnote 39: The same representation and alignment requirements are >>> meant to imply interchangeability as arguments to >>> functions, return values from functions, and members of >>> unions. >>> -^- >>> >>> It does not extend to the "+" operator. >> >> GNU C specifically allows arithmetic on pointers and defines the size >> of a void as 1. I'll comply, but this makes me want to stab myself in >> the face :-\ Thanks, > > We rely on GNU C extensions all over theplace. Making the code uglier > to avoid relying on this one here makes no sense to me. > I agree, in fact. If GNU-isms are liberally used & tolerated in the QEMU source, then there's no reason to diverge from that here. I steer clear of GNU-isms as much as I can, regardless of codebase, but I *did* forget that QEMU permits GNU-isms -- so there's no need for my pedantry here. Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-11 1:36 ` Alex Williamson 2020-03-11 7:08 ` Markus Armbruster @ 2020-03-11 10:26 ` Laszlo Ersek 2020-03-11 11:54 ` Markus Armbruster 1 sibling, 1 reply; 23+ messages in thread From: Laszlo Ersek @ 2020-03-11 10:26 UTC (permalink / raw) To: Alex Williamson Cc: weifuqiang, mst, qemu-devel, Markus Armbruster, arei.gonglei, huangzhichao, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) On 03/11/20 02:36, Alex Williamson wrote: > On Wed, 11 Mar 2020 00:14:31 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 03/10/20 17:11, Alex Williamson wrote: >> >>> commit 2088fc1e1f426b98e9ca4d7bcdbe53d886a18c37 >>> Author: Alex Williamson <alex.williamson@redhat.com> >>> Date: Tue Mar 10 10:04:36 2020 -0600 >>> >>> vfio/pci: Use defined memcpy() behavior >>> >>> vfio_rom_read() relies on memcpy() doing the logically correct thing, >>> ie. safely copying zero bytes from a NULL pointer when rom_size is >>> zero, rather than the spec definition, which is undefined when the >>> source or target pointers are NULL. Resolve this by wrapping the >>> call in the condition expressed previously by the ternary. >>> >>> Additionally, we still use @val to fill data based on the provided >>> @size regardless of mempcy(), so we should initialize @val rather >>> than @data. >>> >>> Reported-by: Longpeng <longpeng2@huawei.com> >>> Reported-by: Laszlo Ersek <lersek@redhat.com> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> >>> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index 5e75a95129ac..b0799cdc28ad 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -859,16 +859,17 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) >>> uint16_t word; >>> uint32_t dword; >>> uint64_t qword; >>> - } val; >>> - uint64_t data = 0; >>> + } val = { 0 }; >>> + uint64_t data; >>> >>> /* Load the ROM lazily when the guest tries to read it */ >>> if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { >>> vfio_pci_load_rom(vdev); >>> } >>> >>> - memcpy(&val, vdev->rom + addr, >>> - (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); >>> + if (addr < vdev->rom_size) { >>> + memcpy(&val, vdev->rom + addr, MIN(size, vdev->rom_size - addr)); >>> + } >>> >>> switch (size) { >>> case 1: >> >> Regarding the pre-patch code: >> >> My understanding is that the memcpy() could be reached with a >> guest-originated "addr" even if "vdev->rom" was NULL. If that's the >> case, then the pre-patch code invokes undefined behavior regardless of >> memcpy(), because it performs pointer arithmetic on a null pointer (not >> to mention that the type of that pointer is (void *)....) >> >> Regarding the proposed change: >> >> (addr < vdev->rom_size) requires that "vdev->rom_size" be positive. In >> that case, I assume that >> >> - "vdev->rom" is not NULL, and >> - MIN(size, vdev->rom_size - addr) bytes are "in range" for the object >> allocated at "vdev->rom". >> >> So from a memcpy() and range perspective, the patch looks OK. But >> there's still a wart I dislike: we should never perform pointer >> arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or >> (unsigned char*) first. >> >> Here's an excerpt from the ISO C99 standard: >> >> -v- >> 6.5.6 Additive operators >> >> Constraints >> >> 2 For addition, either both operands shall have arithmetic type, or one >> operand shall be a pointer to an object type and the other shall have >> integer type. [...] >> -^- >> >> A "pointer-to-void" is not a "pointer to an object type", because "void" >> is not an object type -- it is an incomplete type that cannot be completed: >> >> -v- >> 6.2.5 Types >> >> 1 [...] Types are partitioned into object types (types that fully >> describe objects), function types (types that describe functions), and >> incomplete types (types that describe objects but lack information >> needed to determine their sizes). >> >> [...] >> >> 19 The void type comprises an empty set of values; it is an incomplete >> type that cannot be completed. >> -^- >> >> For a different illustration, (vdev->rom + addr) is equivalent to >> &(vdev->rom[addr]) -- and we clearly can't have an "array of void". >> >> This anti-pattern (of doing pointer arithmetic on (void*)) likely comes >> from a guarantee that the standard does make, in the same "6.2.5 Types" >> section: >> >> -v- >> 27 A pointer to void shall have the same representation and alignment >> requirements as a pointer to a character type. 39) [...] >> >> Footnote 39: The same representation and alignment requirements are >> meant to imply interchangeability as arguments to >> functions, return values from functions, and members of >> unions. >> -^- >> >> It does not extend to the "+" operator. > > GNU C specifically allows arithmetic on pointers and defines the size > of a void as 1. I'll comply, but this makes me want to stab myself in > the face :-\ Thanks, Sorry, I didn't want to annoy you. :) In fact I was about to mention, "I really don't understand why compilers don't yell upon seeing pointer-to-void arithmetic", but I got distracted and forgot about that thought. In retrospect, that may have been for the best! :) Thanks Laszlo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-11 10:26 ` Laszlo Ersek @ 2020-03-11 11:54 ` Markus Armbruster 2020-03-11 13:00 ` Laszlo Ersek 0 siblings, 1 reply; 23+ messages in thread From: Markus Armbruster @ 2020-03-11 11:54 UTC (permalink / raw) To: Laszlo Ersek Cc: mst, weifuqiang, qemu-devel, Alex Williamson, arei.gonglei, huangzhichao, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) Laszlo Ersek <lersek@redhat.com> writes: > On 03/11/20 02:36, Alex Williamson wrote: >> On Wed, 11 Mar 2020 00:14:31 +0100 >> Laszlo Ersek <lersek@redhat.com> wrote: >> >>> On 03/10/20 17:11, Alex Williamson wrote: >>> >>>> commit 2088fc1e1f426b98e9ca4d7bcdbe53d886a18c37 >>>> Author: Alex Williamson <alex.williamson@redhat.com> >>>> Date: Tue Mar 10 10:04:36 2020 -0600 >>>> >>>> vfio/pci: Use defined memcpy() behavior >>>> >>>> vfio_rom_read() relies on memcpy() doing the logically correct thing, >>>> ie. safely copying zero bytes from a NULL pointer when rom_size is >>>> zero, rather than the spec definition, which is undefined when the >>>> source or target pointers are NULL. Resolve this by wrapping the >>>> call in the condition expressed previously by the ternary. >>>> >>>> Additionally, we still use @val to fill data based on the provided >>>> @size regardless of mempcy(), so we should initialize @val rather >>>> than @data. >>>> >>>> Reported-by: Longpeng <longpeng2@huawei.com> >>>> Reported-by: Laszlo Ersek <lersek@redhat.com> >>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> >>>> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index 5e75a95129ac..b0799cdc28ad 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -859,16 +859,17 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) >>>> uint16_t word; >>>> uint32_t dword; >>>> uint64_t qword; >>>> - } val; >>>> - uint64_t data = 0; >>>> + } val = { 0 }; >>>> + uint64_t data; >>>> >>>> /* Load the ROM lazily when the guest tries to read it */ >>>> if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { >>>> vfio_pci_load_rom(vdev); >>>> } >>>> >>>> - memcpy(&val, vdev->rom + addr, >>>> - (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); >>>> + if (addr < vdev->rom_size) { >>>> + memcpy(&val, vdev->rom + addr, MIN(size, vdev->rom_size - addr)); >>>> + } >>>> >>>> switch (size) { >>>> case 1: >>> >>> Regarding the pre-patch code: >>> >>> My understanding is that the memcpy() could be reached with a >>> guest-originated "addr" even if "vdev->rom" was NULL. If that's the >>> case, then the pre-patch code invokes undefined behavior regardless of >>> memcpy(), because it performs pointer arithmetic on a null pointer (not >>> to mention that the type of that pointer is (void *)....) >>> >>> Regarding the proposed change: >>> >>> (addr < vdev->rom_size) requires that "vdev->rom_size" be positive. In >>> that case, I assume that >>> >>> - "vdev->rom" is not NULL, and >>> - MIN(size, vdev->rom_size - addr) bytes are "in range" for the object >>> allocated at "vdev->rom". >>> >>> So from a memcpy() and range perspective, the patch looks OK. But >>> there's still a wart I dislike: we should never perform pointer >>> arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or >>> (unsigned char*) first. >>> >>> Here's an excerpt from the ISO C99 standard: >>> >>> -v- >>> 6.5.6 Additive operators >>> >>> Constraints >>> >>> 2 For addition, either both operands shall have arithmetic type, or one >>> operand shall be a pointer to an object type and the other shall have >>> integer type. [...] >>> -^- >>> >>> A "pointer-to-void" is not a "pointer to an object type", because "void" >>> is not an object type -- it is an incomplete type that cannot be completed: >>> >>> -v- >>> 6.2.5 Types >>> >>> 1 [...] Types are partitioned into object types (types that fully >>> describe objects), function types (types that describe functions), and >>> incomplete types (types that describe objects but lack information >>> needed to determine their sizes). >>> >>> [...] >>> >>> 19 The void type comprises an empty set of values; it is an incomplete >>> type that cannot be completed. >>> -^- >>> >>> For a different illustration, (vdev->rom + addr) is equivalent to >>> &(vdev->rom[addr]) -- and we clearly can't have an "array of void". >>> >>> This anti-pattern (of doing pointer arithmetic on (void*)) likely comes >>> from a guarantee that the standard does make, in the same "6.2.5 Types" >>> section: >>> >>> -v- >>> 27 A pointer to void shall have the same representation and alignment >>> requirements as a pointer to a character type. 39) [...] >>> >>> Footnote 39: The same representation and alignment requirements are >>> meant to imply interchangeability as arguments to >>> functions, return values from functions, and members of >>> unions. >>> -^- >>> >>> It does not extend to the "+" operator. >> >> GNU C specifically allows arithmetic on pointers and defines the size >> of a void as 1. I'll comply, but this makes me want to stab myself in >> the face :-\ Thanks, > > Sorry, I didn't want to annoy you. :) > > In fact I was about to mention, "I really don't understand why compilers > don't yell upon seeing pointer-to-void arithmetic", but I got distracted > and forgot about that thought. In retrospect, that may have been for the > best! :) You're looking for '-Wpointer-arith' Warn about anything that depends on the "size of" a function type or of 'void'. GNU C assigns these types a size of 1, for convenience in calculations with 'void *' pointers and pointers to functions. In C++, warn also when an arithmetic operation involves 'NULL'. This warning is also enabled by '-Wpedantic'. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-11 11:54 ` Markus Armbruster @ 2020-03-11 13:00 ` Laszlo Ersek 0 siblings, 0 replies; 23+ messages in thread From: Laszlo Ersek @ 2020-03-11 13:00 UTC (permalink / raw) To: Markus Armbruster Cc: mst, weifuqiang, qemu-devel, Alex Williamson, arei.gonglei, huangzhichao, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) On 03/11/20 12:54, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: >> In fact I was about to mention, "I really don't understand why compilers >> don't yell upon seeing pointer-to-void arithmetic", but I got distracted >> and forgot about that thought. In retrospect, that may have been for the >> best! :) > > You're looking for > > '-Wpointer-arith' > Warn about anything that depends on the "size of" a function type > or of 'void'. GNU C assigns these types a size of 1, for > convenience in calculations with 'void *' pointers and pointers to > functions. In C++, warn also when an arithmetic operation involves > 'NULL'. This warning is also enabled by '-Wpedantic'. > Thanks! It seems like "-Wpedantic" and "-pedantic" are synonymous. And the latter used to be part of my standard set of flags, while I worked on hosted C programs where I could influence the build flags ;) Cheers, Laszlo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-02-24 16:04 ` Alex Williamson 2020-02-24 23:48 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-11 7:04 ` Markus Armbruster [not found] ` <20200311093939.494bfe27@w520.home> 1 sibling, 1 reply; 23+ messages in thread From: Markus Armbruster @ 2020-03-11 7:04 UTC (permalink / raw) To: Alex Williamson Cc: mst, weifuqiang, qemu-devel, arei.gonglei, huangzhichao, Longpeng(Mike) Alex Williamson <alex.williamson@redhat.com> writes: > On Mon, 24 Feb 2020 14:42:17 +0800 > "Longpeng(Mike)" <longpeng2@huawei.com> wrote: > >> From: Longpeng <longpeng2@huawei.com> >> >> vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in >> some situation (though I've not encountered yet), maybe we should >> avoid the VM abort. What "VM abort" exactly? >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> >> --- >> hw/vfio/pci.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 5e75a95..ed798ae 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) >> } >> } >> >> -static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev) >> { >> struct vfio_region_info *reg_info; >> uint64_t size; >> @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> if (vfio_get_region_info(&vdev->vbasedev, >> VFIO_PCI_ROM_REGION_INDEX, ®_info)) { >> error_report("vfio: Error getting ROM info: %m"); >> - return; >> + return false; >> } >> >> trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned long)reg_info->size, >> @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> error_printf("Device option ROM contents are probably invalid " >> "(check dmesg).\nSkip option ROM probe with rombar=0, " >> "or load from file with romfile=\n"); >> - return; >> + return false; >> } >> >> vdev->rom = g_malloc(size); >> @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> data[6] = -csum; >> } >> } >> + >> + return true; >> } >> >> static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) >> @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) { VFIOPCIDevice *vdev = opaque; union { uint8_t byte; uint16_t word; uint32_t dword; uint64_t qword; } val; >> uint64_t data = 0; >> >> /* Load the ROM lazily when the guest tries to read it */ >> - if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { >> - vfio_pci_load_rom(vdev); >> + if (unlikely(!vdev->rom && !vdev->rom_read_failed) && >> + !vfio_pci_load_rom(vdev)) { >> + return 0; >> } >> >> memcpy(&val, vdev->rom + addr, > > Looks like an obvious bug, until you look at the rest of this memcpy(): > > memcpy(&val, vdev->rom + addr, > (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); > > IOW, we'll do a zero sized memcpy() if rom_size is zero, so there's no > risk of the concern identified in the commit log. This patch is > unnecessary. Thanks, I'm blind: why does !vdev->rom imply !vdev->rom_size? Moreover, when MIN(size, vdev->rom_size - addr) < size, we seem to read uninitialized data from @val: switch (size) { case 1: data = val.byte; break; case 2: data = le16_to_cpu(val.word); break; case 4: data = le32_to_cpu(val.dword); break; default: hw_error("vfio: unsupported read size, %d bytes\n", size); break; } trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data); return data; } Why is that okay? Why do we initialize @data? How can we get to the default case? If we can get there, is hw_error() really the right thing to do? It almost never is... If getting there is the guest's fault, we need to tell it off the same way physical hardware does. If we should not ever get there (i.e. it's a QEMU bug), then a plain abort() would be clearer. ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <20200311093939.494bfe27@w520.home>]
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read [not found] ` <20200311093939.494bfe27@w520.home> @ 2020-03-12 5:50 ` Markus Armbruster 2020-03-12 14:07 ` Alex Williamson 0 siblings, 1 reply; 23+ messages in thread From: Markus Armbruster @ 2020-03-12 5:50 UTC (permalink / raw) To: Alex Williamson Cc: weifuqiang, mst, qemu-devel, arei.gonglei, huangzhichao, Longpeng(Mike) Alex Williamson <alex.williamson@redhat.com> writes: > On Wed, 11 Mar 2020 08:04:28 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Alex Williamson <alex.williamson@redhat.com> writes: >> >> > On Mon, 24 Feb 2020 14:42:17 +0800 >> > "Longpeng(Mike)" <longpeng2@huawei.com> wrote: >> > >> >> From: Longpeng <longpeng2@huawei.com> >> >> >> >> vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in >> >> some situation (though I've not encountered yet), maybe we should >> >> avoid the VM abort. >> >> What "VM abort" exactly? > > There is none because memcpy() does something sane when size is zero, > but to be ISO whatever spec compliant we shouldn't rely on that. > >> >> >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> >> >> --- >> >> hw/vfio/pci.c | 13 ++++++++----- >> >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> >> index 5e75a95..ed798ae 100644 >> >> --- a/hw/vfio/pci.c >> >> +++ b/hw/vfio/pci.c >> >> @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) >> >> } >> >> } >> >> >> >> -static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> >> +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev) >> >> { >> >> struct vfio_region_info *reg_info; >> >> uint64_t size; >> >> @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> >> if (vfio_get_region_info(&vdev->vbasedev, >> >> VFIO_PCI_ROM_REGION_INDEX, ®_info)) { >> >> error_report("vfio: Error getting ROM info: %m"); >> >> - return; >> >> + return false; >> >> } >> >> >> >> trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned long)reg_info->size, >> >> @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> >> error_printf("Device option ROM contents are probably invalid " >> >> "(check dmesg).\nSkip option ROM probe with rombar=0, " >> >> "or load from file with romfile=\n"); >> >> - return; >> >> + return false; >> >> } >> >> >> >> vdev->rom = g_malloc(size); >> >> @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >> >> data[6] = -csum; >> >> } >> >> } >> >> + >> >> + return true; >> >> } >> >> >> >> static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) >> >> @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) >> { >> VFIOPCIDevice *vdev = opaque; >> union { >> uint8_t byte; >> uint16_t word; >> uint32_t dword; >> uint64_t qword; >> } val; >> >> uint64_t data = 0; >> >> >> >> /* Load the ROM lazily when the guest tries to read it */ >> >> - if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { >> >> - vfio_pci_load_rom(vdev); >> >> + if (unlikely(!vdev->rom && !vdev->rom_read_failed) && >> >> + !vfio_pci_load_rom(vdev)) { >> >> + return 0; >> >> } >> >> >> >> memcpy(&val, vdev->rom + addr, >> > >> > Looks like an obvious bug, until you look at the rest of this memcpy(): >> > >> > memcpy(&val, vdev->rom + addr, >> > (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); >> > >> > IOW, we'll do a zero sized memcpy() if rom_size is zero, so there's no >> > risk of the concern identified in the commit log. This patch is >> > unnecessary. Thanks, >> >> I'm blind: why does !vdev->rom imply !vdev->rom_size? > > See vfio_pci_load_rom(), rom_size and rom are set and allocated > together. What if vfio_pci_load_rom() isn't called, or returns before it sets these guys? >> Moreover, when MIN(size, vdev->rom_size - addr) < size, we seem to read >> uninitialized data from @val: > > This is fixed in my patch > https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg02778.html Yes. >> >> switch (size) { >> case 1: >> data = val.byte; >> break; >> case 2: >> data = le16_to_cpu(val.word); >> break; >> case 4: >> data = le32_to_cpu(val.dword); >> break; >> default: >> hw_error("vfio: unsupported read size, %d bytes\n", size); >> break; >> } >> >> trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data); >> >> return data; >> } >> >> Why is that okay? >> >> Why do we initialize @data? > > Bug. The switch was only added later (75bd0c7253f3) and we failed to > catch it. Prior to that we were initializing val and the memcpy() only > overwrote it as necessary. In any case, getting back garbage for the > rom when there isn't one generally works ok since the chances of > generating a proper rom signature are infinitesimal. Clearly not what > was intended though. > >> How can we get to the default case? If we can get there, is hw_error() >> really the right thing to do? It almost never is... If getting there >> is the guest's fault, we need to tell it off the same way physical >> hardware does. If we should not ever get there (i.e. it's a QEMU bug), >> then a plain abort() would be clearer. > > AFAIK this is relatively standard, if not somewhat paranoid, handling > for a MemoryRegion ops callback. The MemoryRegionOps code only allows > certain size accesses, so it would effectively be an internal error to > hit the default case, which seems to be not an uncommon use case of > hw_error. Thanks, Using hw_error() for such programming errors is not helpful. Everything it adds to abort() is useless or misleading. In fact, most uses of hw_error() are not helpful. But you're going with the flow here. I accept that. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read 2020-03-12 5:50 ` Markus Armbruster @ 2020-03-12 14:07 ` Alex Williamson 0 siblings, 0 replies; 23+ messages in thread From: Alex Williamson @ 2020-03-12 14:07 UTC (permalink / raw) To: Markus Armbruster Cc: weifuqiang, mst, qemu-devel, arei.gonglei, huangzhichao, Longpeng(Mike) On Thu, 12 Mar 2020 06:50:30 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Alex Williamson <alex.williamson@redhat.com> writes: > > > On Wed, 11 Mar 2020 08:04:28 +0100 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Alex Williamson <alex.williamson@redhat.com> writes: > >> > >> > On Mon, 24 Feb 2020 14:42:17 +0800 > >> > "Longpeng(Mike)" <longpeng2@huawei.com> wrote: > >> > > >> >> From: Longpeng <longpeng2@huawei.com> > >> >> > >> >> vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in > >> >> some situation (though I've not encountered yet), maybe we should > >> >> avoid the VM abort. > >> > >> What "VM abort" exactly? > > > > There is none because memcpy() does something sane when size is zero, > > but to be ISO whatever spec compliant we shouldn't rely on that. > > > >> >> > >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> > >> >> --- > >> >> hw/vfio/pci.c | 13 ++++++++----- > >> >> 1 file changed, 8 insertions(+), 5 deletions(-) > >> >> > >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > >> >> index 5e75a95..ed798ae 100644 > >> >> --- a/hw/vfio/pci.c > >> >> +++ b/hw/vfio/pci.c > >> >> @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev) > >> >> } > >> >> } > >> >> > >> >> -static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> >> +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> >> { > >> >> struct vfio_region_info *reg_info; > >> >> uint64_t size; > >> >> @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> >> if (vfio_get_region_info(&vdev->vbasedev, > >> >> VFIO_PCI_ROM_REGION_INDEX, ®_info)) { > >> >> error_report("vfio: Error getting ROM info: %m"); > >> >> - return; > >> >> + return false; > >> >> } > >> >> > >> >> trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned long)reg_info->size, > >> >> @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> >> error_printf("Device option ROM contents are probably invalid " > >> >> "(check dmesg).\nSkip option ROM probe with rombar=0, " > >> >> "or load from file with romfile=\n"); > >> >> - return; > >> >> + return false; > >> >> } > >> >> > >> >> vdev->rom = g_malloc(size); > >> >> @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > >> >> data[6] = -csum; > >> >> } > >> >> } > >> >> + > >> >> + return true; > >> >> } > >> >> > >> >> static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > >> >> @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size) > >> { > >> VFIOPCIDevice *vdev = opaque; > >> union { > >> uint8_t byte; > >> uint16_t word; > >> uint32_t dword; > >> uint64_t qword; > >> } val; > >> >> uint64_t data = 0; > >> >> > >> >> /* Load the ROM lazily when the guest tries to read it */ > >> >> - if (unlikely(!vdev->rom && !vdev->rom_read_failed)) { > >> >> - vfio_pci_load_rom(vdev); > >> >> + if (unlikely(!vdev->rom && !vdev->rom_read_failed) && > >> >> + !vfio_pci_load_rom(vdev)) { > >> >> + return 0; > >> >> } > >> >> > >> >> memcpy(&val, vdev->rom + addr, > >> > > >> > Looks like an obvious bug, until you look at the rest of this memcpy(): > >> > > >> > memcpy(&val, vdev->rom + addr, > >> > (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0); > >> > > >> > IOW, we'll do a zero sized memcpy() if rom_size is zero, so there's no > >> > risk of the concern identified in the commit log. This patch is > >> > unnecessary. Thanks, > >> > >> I'm blind: why does !vdev->rom imply !vdev->rom_size? > > > > See vfio_pci_load_rom(), rom_size and rom are set and allocated > > together. > > What if vfio_pci_load_rom() isn't called, or returns before it sets > these guys? vfio_pci_load_rom() is called from this read function if they're not set and we haven't triggered a read failure. They're zero allocated. The intention is that any vfio_pci_load_rom() failure will leave these unset and trigger a zero sized memcpy, or now skip the mempcy as in the patch below. Thanks, Alex > >> Moreover, when MIN(size, vdev->rom_size - addr) < size, we seem to read > >> uninitialized data from @val: > > > > This is fixed in my patch > > https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg02778.html > > Yes. > > >> > >> switch (size) { > >> case 1: > >> data = val.byte; > >> break; > >> case 2: > >> data = le16_to_cpu(val.word); > >> break; > >> case 4: > >> data = le32_to_cpu(val.dword); > >> break; > >> default: > >> hw_error("vfio: unsupported read size, %d bytes\n", size); > >> break; > >> } > >> > >> trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data); > >> > >> return data; > >> } > >> > >> Why is that okay? > >> > >> Why do we initialize @data? > > > > Bug. The switch was only added later (75bd0c7253f3) and we failed to > > catch it. Prior to that we were initializing val and the memcpy() only > > overwrote it as necessary. In any case, getting back garbage for the > > rom when there isn't one generally works ok since the chances of > > generating a proper rom signature are infinitesimal. Clearly not what > > was intended though. > > > >> How can we get to the default case? If we can get there, is hw_error() > >> really the right thing to do? It almost never is... If getting there > >> is the guest's fault, we need to tell it off the same way physical > >> hardware does. If we should not ever get there (i.e. it's a QEMU bug), > >> then a plain abort() would be clearer. > > > > AFAIK this is relatively standard, if not somewhat paranoid, handling > > for a MemoryRegion ops callback. The MemoryRegionOps code only allows > > certain size accesses, so it would effectively be an internal error to > > hit the default case, which seems to be not an uncommon use case of > > hw_error. Thanks, > > Using hw_error() for such programming errors is not helpful. Everything > it adds to abort() is useless or misleading. > > In fact, most uses of hw_error() are not helpful. > > But you're going with the flow here. I accept that. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log 2020-02-24 6:42 [PATCH RESEND 0/3] fix some warnings by static code scan tool Longpeng(Mike) 2020-02-24 6:42 ` [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read Longpeng(Mike) @ 2020-02-24 6:42 ` Longpeng(Mike) 2020-03-10 2:11 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-10 5:57 ` Michael S. Tsirkin 2020-02-24 6:42 ` [PATCH RESEND 3/3] util/pty: fix a null pointer reference in qemu_openpty_raw Longpeng(Mike) 2 siblings, 2 replies; 23+ messages in thread From: Longpeng(Mike) @ 2020-02-24 6:42 UTC (permalink / raw) To: mst, alex.williamson Cc: longpeng2, arei.gonglei, huangzhichao, qemu-devel, weifuqiang From: Longpeng <longpeng2@huawei.com> vhost_log_alloc() may fails and returned pointer of log is null. However there're two places derefernce the return pointer without check. Signed-off-by: Longpeng <longpeng2@huawei.com> --- hw/virtio/vhost.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 9edfadc..c7ad6e5 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) if (!log || log->size != size) { log = vhost_log_alloc(size, share); + if (!log) { + return NULL; + } + if (share) { vhost_log_shm = log; } else { @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) { - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); - uint64_t log_base = (uintptr_t)log->log; + struct vhost_log *log; + uint64_t log_base; int r; + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); + if (!log) { + return; + } + + log_base = (uintptr_t)log->log; + /* inform backend of log switching, this must be done before releasing the current log, to ensure no logging is lost */ r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); @@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) hdev->log_size = vhost_get_log_size(hdev); hdev->log = vhost_log_get(hdev->log_size, vhost_dev_log_is_shared(hdev)); + if (!hdev->log) { + goto fail_vq; + } + log_base = (uintptr_t)hdev->log->log; r = hdev->vhost_ops->vhost_set_log_base(hdev, hdev->log_size ? log_base : 0, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log 2020-02-24 6:42 ` [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log Longpeng(Mike) @ 2020-03-10 2:11 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-10 5:57 ` Michael S. Tsirkin 1 sibling, 0 replies; 23+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-10 2:11 UTC (permalink / raw) To: mst; +Cc: arei.gonglei, huangzhichao, qemu-devel, weifuqiang Hi Michael, ping... On 2020/2/24 14:42, Longpeng(Mike) wrote: > From: Longpeng <longpeng2@huawei.com> > > vhost_log_alloc() may fails and returned pointer of log is null. > However there're two places derefernce the return pointer without > check. > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/virtio/vhost.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 9edfadc..c7ad6e5 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) > > if (!log || log->size != size) { > log = vhost_log_alloc(size, share); > + if (!log) { > + return NULL; > + } > + > if (share) { > vhost_log_shm = log; > } else { > @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > > static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > { > - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > - uint64_t log_base = (uintptr_t)log->log; > + struct vhost_log *log; > + uint64_t log_base; > int r; > > + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > + if (!log) { > + return; > + } > + > + log_base = (uintptr_t)log->log; > + > /* inform backend of log switching, this must be done before > releasing the current log, to ensure no logging is lost */ > r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); > @@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) > hdev->log_size = vhost_get_log_size(hdev); > hdev->log = vhost_log_get(hdev->log_size, > vhost_dev_log_is_shared(hdev)); > + if (!hdev->log) { > + goto fail_vq; > + } > + > log_base = (uintptr_t)hdev->log->log; > r = hdev->vhost_ops->vhost_set_log_base(hdev, > hdev->log_size ? log_base : 0, > -- --- Regards, Longpeng(Mike) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log 2020-02-24 6:42 ` [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log Longpeng(Mike) 2020-03-10 2:11 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-10 5:57 ` Michael S. Tsirkin 2020-03-10 8:04 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 1 sibling, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2020-03-10 5:57 UTC (permalink / raw) To: Longpeng(Mike) Cc: weifuqiang, Jason Wang, qemu-devel, alex.williamson, arei.gonglei, huangzhichao, Marc-André Lureau On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote: > From: Longpeng <longpeng2@huawei.com> > > vhost_log_alloc() may fails and returned pointer of log is null. > However there're two places derefernce the return pointer without > check. > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/virtio/vhost.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 9edfadc..c7ad6e5 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) > > if (!log || log->size != size) { > log = vhost_log_alloc(size, share); > + if (!log) { > + return NULL; > + } > + > if (share) { > vhost_log_shm = log; > } else { > @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > > static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > { > - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > - uint64_t log_base = (uintptr_t)log->log; > + struct vhost_log *log; > + uint64_t log_base; > int r; > > + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > + if (!log) { > + return; > + } > + I'm not sure silently failing like this is safe. Callers assume log can be resized. What can be done? I suspect not much beside exiting ... Speaking of which, lots of other failures in log resizing path seem to be silently ignored. I guess we should propagate them, and fix callers to check the return code? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log 2020-03-10 5:57 ` Michael S. Tsirkin @ 2020-03-10 8:04 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-10 8:23 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-10 8:04 UTC (permalink / raw) To: Michael S. Tsirkin Cc: weifuqiang, Jason Wang, qemu-devel, alex.williamson, arei.gonglei, huangzhichao, Marc-André Lureau On 2020/3/10 13:57, Michael S. Tsirkin wrote: > On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote: >> From: Longpeng <longpeng2@huawei.com> >> >> vhost_log_alloc() may fails and returned pointer of log is null. >> However there're two places derefernce the return pointer without >> check. >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> >> --- >> hw/virtio/vhost.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 9edfadc..c7ad6e5 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) >> >> if (!log || log->size != size) { >> log = vhost_log_alloc(size, share); >> + if (!log) { >> + return NULL; >> + } >> + >> if (share) { >> vhost_log_shm = log; >> } else { >> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) >> >> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) >> { >> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); >> - uint64_t log_base = (uintptr_t)log->log; >> + struct vhost_log *log; >> + uint64_t log_base; >> int r; >> >> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); >> + if (!log) { >> + return; >> + } >> + > > I'm not sure silently failing like this is safe. Callers assume > log can be resized. What can be done? I suspect not much > beside exiting ... > Speaking of which, lots of other failures in log resizing > path seem to be silently ignored. > I guess we should propagate them, and fix callers to check > the return code? > How about to let the callers treat the failure of log_resize as a fatal error ? -static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) +static inline int vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) { - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); - uint64_t log_base = (uintptr_t)log->log; + struct vhost_log *log; + uint64_t log_base; int r; + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); + if (!log) { + r = -1; + goto out; + } + + log_base = (uintptr_t)log->log; + /* inform backend of log switching, this must be done before releasing the current log, to ensure no logging is lost */ r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); @@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) vhost_log_put(dev, true); dev->log = log; dev->log_size = size; + +out: + return 0; } @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener) #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log) /* To log more, must increase log size before table update. */ if (dev->log_size < log_size) { - vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER); + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { + abort(); + } } r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem); if (r < 0) { @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener) } /* To log less, can only decrease log size after table update. */ if (dev->log_size > log_size + VHOST_LOG_BUFFER) { - vhost_dev_log_resize(dev, log_size); + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { + abort(); + } } out: @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener, int enable) } vhost_log_put(dev, false); } else { - vhost_dev_log_resize(dev, vhost_get_log_size(dev)); + r = vhost_dev_log_resize(dev, vhost_get_log_size(dev)); + if (r < 0) { + return r; + } + r = vhost_dev_set_log(dev, true); if (r < 0) { return r; > > . > -- --- Regards, Longpeng(Mike) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log 2020-03-10 8:04 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-10 8:23 ` Michael S. Tsirkin 2020-03-10 12:02 ` Longpeng (Mike) 0 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2020-03-10 8:23 UTC (permalink / raw) To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) Cc: weifuqiang, Jason Wang, qemu-devel, alex.williamson, arei.gonglei, huangzhichao, Marc-André Lureau On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > > > On 2020/3/10 13:57, Michael S. Tsirkin wrote: > > On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote: > >> From: Longpeng <longpeng2@huawei.com> > >> > >> vhost_log_alloc() may fails and returned pointer of log is null. > >> However there're two places derefernce the return pointer without > >> check. > >> > >> Signed-off-by: Longpeng <longpeng2@huawei.com> > >> --- > >> hw/virtio/vhost.c | 19 +++++++++++++++++-- > >> 1 file changed, 17 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > >> index 9edfadc..c7ad6e5 100644 > >> --- a/hw/virtio/vhost.c > >> +++ b/hw/virtio/vhost.c > >> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) > >> > >> if (!log || log->size != size) { > >> log = vhost_log_alloc(size, share); > >> + if (!log) { > >> + return NULL; > >> + } > >> + > >> if (share) { > >> vhost_log_shm = log; > >> } else { > >> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > >> > >> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > >> { > >> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > >> - uint64_t log_base = (uintptr_t)log->log; > >> + struct vhost_log *log; > >> + uint64_t log_base; > >> int r; > >> > >> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > >> + if (!log) { > >> + return; > >> + } > >> + > > > > I'm not sure silently failing like this is safe. Callers assume > > log can be resized. What can be done? I suspect not much > > beside exiting ... > > Speaking of which, lots of other failures in log resizing > > path seem to be silently ignored. > > I guess we should propagate them, and fix callers to check > > the return code? > > > How about to let the callers treat the failure of log_resize as a fatal error ? > > -static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > +static inline int vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > { > - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > - uint64_t log_base = (uintptr_t)log->log; > + struct vhost_log *log; > + uint64_t log_base; > int r; > > + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > + if (!log) { > + r = -1; > + goto out; > + } > + > + log_base = (uintptr_t)log->log; > + > /* inform backend of log switching, this must be done before > releasing the current log, to ensure no logging is lost */ > r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); > @@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev > *dev, uint64_t size) > vhost_log_put(dev, true); > dev->log = log; > dev->log_size = size; > + > +out: > + return 0; > } > > > @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener) > #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log) > /* To log more, must increase log size before table update. */ > if (dev->log_size < log_size) { > - vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER); > + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { > + abort(); > + } > } > r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem); > if (r < 0) { > @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener) > } > /* To log less, can only decrease log size after table update. */ > if (dev->log_size > log_size + VHOST_LOG_BUFFER) { > - vhost_dev_log_resize(dev, log_size); > + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { > + abort(); > + } > } > > out: I think the suggested handling is error_report() and exit(). we also need to propagate errno. So how about passing in Error then? > @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener, > int enable) > } > vhost_log_put(dev, false); > } else { > - vhost_dev_log_resize(dev, vhost_get_log_size(dev)); > + r = vhost_dev_log_resize(dev, vhost_get_log_size(dev)); > + if (r < 0) { > + return r; > + } > + > r = vhost_dev_set_log(dev, true); > if (r < 0) { > return r; > > > > > > . > > > > -- > --- > Regards, > Longpeng(Mike) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log 2020-03-10 8:23 ` Michael S. Tsirkin @ 2020-03-10 12:02 ` Longpeng (Mike) 2020-03-10 12:18 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Longpeng (Mike) @ 2020-03-10 12:02 UTC (permalink / raw) To: Michael S. Tsirkin Cc: weifuqiang, Jason Wang, qemu-devel, alex.williamson, arei.gonglei, huangzhichao, Marc-André Lureau 在 2020/3/10 16:23, Michael S. Tsirkin 写道: > On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: >> >> >> On 2020/3/10 13:57, Michael S. Tsirkin wrote: >>> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote: >>>> From: Longpeng <longpeng2@huawei.com> >>>> >>>> vhost_log_alloc() may fails and returned pointer of log is null. >>>> However there're two places derefernce the return pointer without >>>> check. >>>> [...] >>>> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) >>>> { >>>> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); >>>> - uint64_t log_base = (uintptr_t)log->log; >>>> + struct vhost_log *log; >>>> + uint64_t log_base; >>>> int r; >>>> >>>> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); >>>> + if (!log) { >>>> + return; >>>> + } >>>> + >>> >>> I'm not sure silently failing like this is safe. Callers assume >>> log can be resized. What can be done? I suspect not much >>> beside exiting ... >>> Speaking of which, lots of other failures in log resizing >>> path seem to be silently ignored. >>> I guess we should propagate them, and fix callers to check >>> the return code? >>> >> How about to let the callers treat the failure of log_resize as a fatal error ? >> [...] >> >> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener) >> #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log) >> /* To log more, must increase log size before table update. */ >> if (dev->log_size < log_size) { >> - vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER); >> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { >> + abort(); >> + } >> } >> r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem); >> if (r < 0) { >> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener) >> } >> /* To log less, can only decrease log size after table update. */ >> if (dev->log_size > log_size + VHOST_LOG_BUFFER) { >> - vhost_dev_log_resize(dev, log_size); >> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { >> + abort(); >> + } >> } >> >> out: > > > I think the suggested handling is > error_report() and exit(). > we also need to propagate errno. So how about passing in Error then? > vhost_dev_log_resize vhost_log_get vhost_log_alloc error_report_err (fail path, errno is in the errp) VHOST_OPS_DEBUG (if ->vhost_set_log_base fail) error_report (errno) Um, it seems log_resize will report error with errno internal, do we need error_report once more ? > >> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener, >> int enable) >> } >> vhost_log_put(dev, false); >> } else { >> - vhost_dev_log_resize(dev, vhost_get_log_size(dev)); >> + r = vhost_dev_log_resize(dev, vhost_get_log_size(dev)); >> + if (r < 0) { >> + return r; >> + } >> + >> r = vhost_dev_set_log(dev, true); >> if (r < 0) { >> return r; >> >> >>> >>> . >>> >> >> -- >> --- >> Regards, >> Longpeng(Mike) > > > . > -- Regards, Longpeng(Mike) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log 2020-03-10 12:02 ` Longpeng (Mike) @ 2020-03-10 12:18 ` Michael S. Tsirkin 0 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2020-03-10 12:18 UTC (permalink / raw) To: Longpeng (Mike) Cc: weifuqiang, Jason Wang, qemu-devel, alex.williamson, arei.gonglei, huangzhichao, Marc-André Lureau On Tue, Mar 10, 2020 at 08:02:30PM +0800, Longpeng (Mike) wrote: > 在 2020/3/10 16:23, Michael S. Tsirkin 写道: > > On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > >> > >> > >> On 2020/3/10 13:57, Michael S. Tsirkin wrote: > >>> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote: > >>>> From: Longpeng <longpeng2@huawei.com> > >>>> > >>>> vhost_log_alloc() may fails and returned pointer of log is null. > >>>> However there're two places derefernce the return pointer without > >>>> check. > >>>> > [...] > > >>>> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > >>>> { > >>>> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > >>>> - uint64_t log_base = (uintptr_t)log->log; > >>>> + struct vhost_log *log; > >>>> + uint64_t log_base; > >>>> int r; > >>>> > >>>> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > >>>> + if (!log) { > >>>> + return; > >>>> + } > >>>> + > >>> > >>> I'm not sure silently failing like this is safe. Callers assume > >>> log can be resized. What can be done? I suspect not much > >>> beside exiting ... > >>> Speaking of which, lots of other failures in log resizing > >>> path seem to be silently ignored. > >>> I guess we should propagate them, and fix callers to check > >>> the return code? > >>> > >> How about to let the callers treat the failure of log_resize as a fatal error ? > >> > [...] > > >> > >> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener) > >> #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log) > >> /* To log more, must increase log size before table update. */ > >> if (dev->log_size < log_size) { > >> - vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER); > >> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { > >> + abort(); > >> + } > >> } > >> r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem); > >> if (r < 0) { > >> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener) > >> } > >> /* To log less, can only decrease log size after table update. */ > >> if (dev->log_size > log_size + VHOST_LOG_BUFFER) { > >> - vhost_dev_log_resize(dev, log_size); > >> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) { > >> + abort(); > >> + } > >> } > >> > >> out: > > > > > > I think the suggested handling is > > error_report() and exit(). > > we also need to propagate errno. So how about passing in Error then? > > > vhost_dev_log_resize > vhost_log_get > vhost_log_alloc > error_report_err (fail path, errno is in the errp) > VHOST_OPS_DEBUG (if ->vhost_set_log_base fail) > error_report (errno) > > Um, it seems log_resize will report error with errno internal, do we need > error_report once more ? Well we need to convert this over to something other than VHOST_OPS_DEBUG, that will go away at some point. > > > >> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener, > >> int enable) > >> } > >> vhost_log_put(dev, false); > >> } else { > >> - vhost_dev_log_resize(dev, vhost_get_log_size(dev)); > >> + r = vhost_dev_log_resize(dev, vhost_get_log_size(dev)); > >> + if (r < 0) { > >> + return r; > >> + } > >> + > >> r = vhost_dev_set_log(dev, true); > >> if (r < 0) { > >> return r; > >> > >> > >>> > >>> . > >>> > >> > >> -- > >> --- > >> Regards, > >> Longpeng(Mike) > > > > > > . > > > > > -- > Regards, > Longpeng(Mike) ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH RESEND 3/3] util/pty: fix a null pointer reference in qemu_openpty_raw 2020-02-24 6:42 [PATCH RESEND 0/3] fix some warnings by static code scan tool Longpeng(Mike) 2020-02-24 6:42 ` [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read Longpeng(Mike) 2020-02-24 6:42 ` [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log Longpeng(Mike) @ 2020-02-24 6:42 ` Longpeng(Mike) 2 siblings, 0 replies; 23+ messages in thread From: Longpeng(Mike) @ 2020-02-24 6:42 UTC (permalink / raw) To: mst, alex.williamson Cc: longpeng2, arei.gonglei, huangzhichao, qemu-devel, weifuqiang From: Longpeng <longpeng2@huawei.com> q_ptsname may failed ane return null, so use the returned pointer as the param of strcpy will cause null pointer deference. Use the return string of openpty instead of call ptsname. Signed-off-by: Longpeng <longpeng2@huawei.com> --- util/qemu-openpty.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/util/qemu-openpty.c b/util/qemu-openpty.c index 2e8b43b..2bea4ba 100644 --- a/util/qemu-openpty.c +++ b/util/qemu-openpty.c @@ -112,13 +112,7 @@ int qemu_openpty_raw(int *aslave, char *pty_name) { int amaster; struct termios tty; -#if defined(__OpenBSD__) || defined(__DragonFly__) - char pty_buf[PATH_MAX]; -#define q_ptsname(x) pty_buf -#else - char *pty_buf = NULL; -#define q_ptsname(x) ptsname(x) -#endif + char pty_buf[PATH_MAX] = { 0 }; if (openpty(&amaster, aslave, pty_buf, NULL, NULL) < 0) { return -1; @@ -130,7 +124,7 @@ int qemu_openpty_raw(int *aslave, char *pty_name) tcsetattr(*aslave, TCSAFLUSH, &tty); if (pty_name) { - strcpy(pty_name, q_ptsname(amaster)); + strcpy(pty_name, pty_buf); } return amaster; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-03-12 14:08 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-24 6:42 [PATCH RESEND 0/3] fix some warnings by static code scan tool Longpeng(Mike) 2020-02-24 6:42 ` [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read Longpeng(Mike) 2020-02-24 16:04 ` Alex Williamson 2020-02-24 23:48 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-10 16:11 ` Alex Williamson 2020-03-10 23:14 ` Laszlo Ersek 2020-03-11 1:36 ` Alex Williamson 2020-03-11 7:08 ` Markus Armbruster 2020-03-11 10:28 ` Laszlo Ersek 2020-03-11 10:26 ` Laszlo Ersek 2020-03-11 11:54 ` Markus Armbruster 2020-03-11 13:00 ` Laszlo Ersek 2020-03-11 7:04 ` Markus Armbruster [not found] ` <20200311093939.494bfe27@w520.home> 2020-03-12 5:50 ` Markus Armbruster 2020-03-12 14:07 ` Alex Williamson 2020-02-24 6:42 ` [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log Longpeng(Mike) 2020-03-10 2:11 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-10 5:57 ` Michael S. Tsirkin 2020-03-10 8:04 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2020-03-10 8:23 ` Michael S. Tsirkin 2020-03-10 12:02 ` Longpeng (Mike) 2020-03-10 12:18 ` Michael S. Tsirkin 2020-02-24 6:42 ` [PATCH RESEND 3/3] util/pty: fix a null pointer reference in qemu_openpty_raw Longpeng(Mike)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.