| Message ID | 20171028025315.13500-1-hw42@ipsumj.de |
|---|---|
| State | New |
| Headers | show |
| Series | xen/pt: Set is_express to avoid out-of-bounds write | expand |
On Sat, 28 Oct 2017, Simon Gaiser wrote: > The passed-through device might be an express device. In this case the > old code allocated a too small emulated config space in > pci_config_alloc() since pci_config_size() returned the size for a > non-express device. This leads to an out-of-bound write in > xen_pt_config_reg_init(), which sometimes results in crashes. So set > is_express as already done for KVM in vfio-pci. > > Shortened ASan report: > > ==17512==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000041648 at pc 0x55e0fdac51ff bp 0x7ffe4af07410 sp 0x7ffe4af07408 > WRITE of size 2 at 0x611000041648 thread T0 > #0 0x55e0fdac51fe in memcpy /usr/include/x86_64-linux-gnu/bits/string3.h:53 > #1 0x55e0fdac51fe in stw_he_p include/qemu/bswap.h:330 > #2 0x55e0fdac51fe in stw_le_p include/qemu/bswap.h:379 > #3 0x55e0fdac51fe in pci_set_word include/hw/pci/pci.h:490 > #4 0x55e0fdac51fe in xen_pt_config_reg_init hw/xen/xen_pt_config_init.c:1991 > #5 0x55e0fdac51fe in xen_pt_config_init hw/xen/xen_pt_config_init.c:2067 > #6 0x55e0fdabcf4d in xen_pt_realize hw/xen/xen_pt.c:830 > #7 0x55e0fdf59666 in pci_qdev_realize hw/pci/pci.c:2034 > #8 0x55e0fdda7d3d in device_set_realized hw/core/qdev.c:914 > [...] > > 0x611000041648 is located 8 bytes to the right of 256-byte region [0x611000041540,0x611000041640) > allocated by thread T0 here: > #0 0x7ff596a94bb8 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9bb8) > #1 0x7ff57da66580 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x50580) > #2 0x55e0fdda7d3d in device_set_realized hw/core/qdev.c:914 > [...] > > Signed-off-by: Simon Gaiser <hw42@ipsumj.de> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > > I found this by debugging crashes and I'm not familiar with this code, > so I'm not sure if this has no unintended side effects. > > hw/xen/xen_pt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index b6d71bb52a..90ffd45e7d 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -946,6 +946,7 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data) > k->exit = xen_pt_unregister_device; > k->config_read = xen_pt_pci_read_config; > k->config_write = xen_pt_pci_write_config; > + k->is_express = 1; /* We might be */ > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > dc->desc = "Assign an host PCI device with Xen"; > dc->props = xen_pci_passthrough_properties; > -- > 2.15.0.rc1 > >
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index b6d71bb52a..90ffd45e7d 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -946,6 +946,7 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data) k->exit = xen_pt_unregister_device; k->config_read = xen_pt_pci_read_config; k->config_write = xen_pt_pci_write_config; + k->is_express = 1; /* We might be */ set_bit(DEVICE_CATEGORY_MISC, dc->categories); dc->desc = "Assign an host PCI device with Xen"; dc->props = xen_pci_passthrough_properties;
The passed-through device might be an express device. In this case the old code allocated a too small emulated config space in pci_config_alloc() since pci_config_size() returned the size for a non-express device. This leads to an out-of-bound write in xen_pt_config_reg_init(), which sometimes results in crashes. So set is_express as already done for KVM in vfio-pci. Shortened ASan report: ==17512==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000041648 at pc 0x55e0fdac51ff bp 0x7ffe4af07410 sp 0x7ffe4af07408 WRITE of size 2 at 0x611000041648 thread T0 #0 0x55e0fdac51fe in memcpy /usr/include/x86_64-linux-gnu/bits/string3.h:53 #1 0x55e0fdac51fe in stw_he_p include/qemu/bswap.h:330 #2 0x55e0fdac51fe in stw_le_p include/qemu/bswap.h:379 #3 0x55e0fdac51fe in pci_set_word include/hw/pci/pci.h:490 #4 0x55e0fdac51fe in xen_pt_config_reg_init hw/xen/xen_pt_config_init.c:1991 #5 0x55e0fdac51fe in xen_pt_config_init hw/xen/xen_pt_config_init.c:2067 #6 0x55e0fdabcf4d in xen_pt_realize hw/xen/xen_pt.c:830 #7 0x55e0fdf59666 in pci_qdev_realize hw/pci/pci.c:2034 #8 0x55e0fdda7d3d in device_set_realized hw/core/qdev.c:914 [...] 0x611000041648 is located 8 bytes to the right of 256-byte region [0x611000041540,0x611000041640) allocated by thread T0 here: #0 0x7ff596a94bb8 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9bb8) #1 0x7ff57da66580 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x50580) #2 0x55e0fdda7d3d in device_set_realized hw/core/qdev.c:914 [...] Signed-off-by: Simon Gaiser <hw42@ipsumj.de> --- I found this by debugging crashes and I'm not familiar with this code, so I'm not sure if this has no unintended side effects. hw/xen/xen_pt.c | 1 + 1 file changed, 1 insertion(+)