From cf2f89edf36a59183166ae8721a8d7ab5cd286bd Mon Sep 17 00:00:00 2001 From: Eric Auger Date: Mon, 17 Jul 2023 18:21:26 +0200 Subject: hw/virtio-iommu: Fix potential OOB access in virtio_iommu_handle_command() In the virtio_iommu_handle_command() when a PROBE request is handled, output_size takes a value greater than the tail size and on a subsequent iteration we can get a stack out-of-band access. Initialize the output_size on each iteration. The issue was found with ASAN. Credits to: Yiming Tao(Zhejiang University) Gaoning Pan(Zhejiang University) Fixes: 1733eebb9e7 ("virtio-iommu: Implement RESV_MEM probe request") Signed-off-by: Eric Auger Reported-by: Mauro Matteo Cascella Cc: qemu-stable@nongnu.org Message-Id: <20230717162126.11693-1-eric.auger@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio-iommu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 201127c..4dcf1d5 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -728,13 +728,15 @@ static void virtio_iommu_handle_command(VirtIODevice *vdev, VirtQueue *vq) VirtIOIOMMU *s = VIRTIO_IOMMU(vdev); struct virtio_iommu_req_head head; struct virtio_iommu_req_tail tail = {}; - size_t output_size = sizeof(tail), sz; VirtQueueElement *elem; unsigned int iov_cnt; struct iovec *iov; void *buf = NULL; + size_t sz; for (;;) { + size_t output_size = sizeof(tail); + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); if (!elem) { return; -- cgit v1.1 From 503d86dd66625b4bed9484bca71db1678c730dc9 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 18 Jul 2023 11:13:27 +0100 Subject: hw/pci-bridge/cxl_upstream.c: Use g_new0() in build_cdat_table() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In build_cdat_table() we do: *cdat_table = g_malloc0(sizeof(*cdat_table) * CXL_USP_CDAT_NUM_ENTRIES); This is wrong because: - cdat_table has type CDATSubHeader *** - so *cdat_table has type CDATSubHeader ** - so the array we're allocating here should be items of type CDATSubHeader * - but we pass sizeof(*cdat_table), which is sizeof(CDATSubHeader **), implying that we're allocating an array of CDATSubHeader ** It happens that sizeof(CDATSubHeader **) == sizeof(CDATSubHeader *) so nothing blows up, but this should be sizeof(**cdat_table). Avoid this excessively hard-to-understand code by using g_new0() instead, which will do the type checking for us. While we're here, we can drop the useless check against failure, as g_malloc0() and g_new0() never fail. This fixes Coverity issue CID 1508120. Signed-off-by: Peter Maydell Message-Id: <20230718101327.1111374-1-peter.maydell@linaro.org> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Jonathan Cameron --- hw/pci-bridge/cxl_upstream.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'hw') diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c index ef47e5d..9159f48 100644 --- a/hw/pci-bridge/cxl_upstream.c +++ b/hw/pci-bridge/cxl_upstream.c @@ -274,10 +274,7 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv) }; } - *cdat_table = g_malloc0(sizeof(*cdat_table) * CXL_USP_CDAT_NUM_ENTRIES); - if (!*cdat_table) { - return -ENOMEM; - } + *cdat_table = g_new0(CDATSubHeader *, CXL_USP_CDAT_NUM_ENTRIES); /* Header always at start of structure */ (*cdat_table)[CXL_USP_CDAT_SSLBIS_LAT] = g_steal_pointer(&sslbis_latency); -- cgit v1.1 From 1084feddc6a677cdfdde56936bfb97cf32cc4dee Mon Sep 17 00:00:00 2001 From: Eric Auger Date: Tue, 18 Jul 2023 20:21:36 +0200 Subject: virtio-iommu: Standardize granule extraction and formatting At several locations we compute the granule from the config page_size_mask using ctz() and then format it in traces using BIT(). As the page_size_mask is 64b we should use ctz64 and BIT_ULL() for formatting. We failed to be consistent. Note the page_size_mask is garanteed to be non null. The spec mandates the device to set at least one bit, so ctz64 cannot return 64. This is garanteed by the fact the device initializes the page_size_mask to qemu_target_page_mask() and then the page_size_mask is further constrained by virtio_iommu_set_page_size_mask() callback which can't result in a new mask being null. So if Coverity complains round those ctz64/BIT_ULL with CID 1517772 this is a false positive Signed-off-by: Eric Auger Fixes: 94df5b2180 ("virtio-iommu: Fix 64kB host page size VFIO device assignment") Message-Id: <20230718182136.40096-1-eric.auger@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Jean-Philippe Brucker --- hw/virtio/virtio-iommu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'hw') diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 4dcf1d5..be51635 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -854,17 +854,19 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr, VirtIOIOMMUEndpoint *ep; uint32_t sid, flags; bool bypass_allowed; + int granule; bool found; int i; interval.low = addr; interval.high = addr + 1; + granule = ctz64(s->config.page_size_mask); IOMMUTLBEntry entry = { .target_as = &address_space_memory, .iova = addr, .translated_addr = addr, - .addr_mask = (1 << ctz32(s->config.page_size_mask)) - 1, + .addr_mask = BIT_ULL(granule) - 1, .perm = IOMMU_NONE, }; @@ -1117,7 +1119,7 @@ static int virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr, if (s->granule_frozen) { int cur_granule = ctz64(cur_mask); - if (!(BIT(cur_granule) & new_mask)) { + if (!(BIT_ULL(cur_granule) & new_mask)) { error_setg(errp, "virtio-iommu %s does not support frozen granule 0x%llx", mr->parent_obj.name, BIT_ULL(cur_granule)); return -1; @@ -1163,7 +1165,7 @@ static void virtio_iommu_freeze_granule(Notifier *notifier, void *data) } s->granule_frozen = true; granule = ctz64(s->config.page_size_mask); - trace_virtio_iommu_freeze_granule(BIT(granule)); + trace_virtio_iommu_freeze_granule(BIT_ULL(granule)); } static void virtio_iommu_device_realize(DeviceState *dev, Error **errp) -- cgit v1.1 From 63a3520e29a1a68d8610315b049ccb5840fe22e9 Mon Sep 17 00:00:00 2001 From: Milan Zamazal Date: Thu, 20 Jul 2023 12:10:37 +0200 Subject: hw/virtio: Add a protection against duplicate vu_scmi_stop calls The QEMU CI fails in virtio-scmi test occasionally. As reported by Thomas Huth, this happens most likely when the system is loaded and it fails with the following error: qemu-system-aarch64: ../../devel/qemu/hw/pci/msix.c:659: msix_unset_vector_notifiers: Assertion `dev->msix_vector_use_notifier && dev->msix_vector_release_notifier' failed. ../../devel/qemu/tests/qtest/libqtest.c:200: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped) As discovered by Fabiano Rosas, the cause is a duplicate invocation of msix_unset_vector_notifiers via duplicate vu_scmi_stop calls: msix_unset_vector_notifiers virtio_pci_set_guest_notifiers vu_scmi_stop vu_scmi_disconnect ... qemu_chr_write_buffer msix_unset_vector_notifiers virtio_pci_set_guest_notifiers vu_scmi_stop vu_scmi_set_status ... qemu_cleanup While vu_scmi_stop calls are protected by vhost_dev_is_started() check, it's apparently not enough. vhost-user-blk and vhost-user-gpio use an extra protection, see f5b22d06fb (vhost: recheck dev state in the vhost_migration_log routine) for the motivation. Let's use the same in vhost-user-scmi, which fixes the failure above. Fixes: a5dab090e142 ("hw/virtio: Add boilerplate for vhost-user-scmi device") Signed-off-by: Milan Zamazal Message-Id: <20230720101037.2161450-1-mzamazal@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Tested-by: Thomas Huth Reviewed-by: Fabiano Rosas --- hw/virtio/vhost-user-scmi.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'hw') diff --git a/hw/virtio/vhost-user-scmi.c b/hw/virtio/vhost-user-scmi.c index d386fb2..918bb7d 100644 --- a/hw/virtio/vhost-user-scmi.c +++ b/hw/virtio/vhost-user-scmi.c @@ -63,6 +63,7 @@ static int vu_scmi_start(VirtIODevice *vdev) error_report("Error starting vhost-user-scmi: %d", ret); goto err_guest_notifiers; } + scmi->started_vu = true; /* * guest_notifier_mask/pending not used yet, so just unmask @@ -90,6 +91,12 @@ static void vu_scmi_stop(VirtIODevice *vdev) struct vhost_dev *vhost_dev = &scmi->vhost_dev; int ret; + /* vhost_dev_is_started() check in the callers is not fully reliable. */ + if (!scmi->started_vu) { + return; + } + scmi->started_vu = false; + if (!k->set_guest_notifiers) { return; } -- cgit v1.1 From 44d975ef340e2f21f236f9520c53e1b30d2213a4 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 20 Jul 2023 15:38:54 +0200 Subject: x86: acpi: workaround Windows not handling name references in Package properly it seems that Windows is unable to handle variable references making it choke up when accessing ASUN during _DSM call when device is hotplugged (it lists package elements as DataAlias but despite that later on it misbehaves) with following error shown up in AMLI debugger (WS2012r2): Store(ShiftLeft(One,Arg1="ASUN",) AMLI_ERROR(c0140008): Unexpected argument type ValidateArgTypes: expected Arg1 to be type Integer (Type=String) Similar outcome with WS2022. Issue is not fatal but as result acpi-index/"PCI Label ID" property is either not shown in device details page or shows incorrect value. Fix it by doing assignment of BSEL/ASUN values to package elements manually after package declaration. Fix was tested with: WS2012r2, WS2022, RHEL9 Fixes: 467d099a2985 (x86: acpi: _DSM: use Package to pass parameters) Signed-off-by: Igor Mammedov Message-Id: <20230720133858.1974024-3-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 9c74fa1..19d268f 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -362,10 +362,14 @@ Aml *aml_pci_device_dsm(void) { Aml *params = aml_local(0); Aml *pkg = aml_package(2); - aml_append(pkg, aml_name("BSEL")); - aml_append(pkg, aml_name("ASUN")); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_int(0)); aml_append(method, aml_store(pkg, params)); aml_append(method, + aml_store(aml_name("BSEL"), aml_index(params, aml_int(0)))); + aml_append(method, + aml_store(aml_name("ASUN"), aml_index(params, aml_int(1)))); + aml_append(method, aml_return(aml_call5("PDSM", aml_arg(0), aml_arg(1), aml_arg(2), aml_arg(3), params)) ); -- cgit v1.1 From 5ce869f788b0b8d82693212366d5637d9f3206c9 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 20 Jul 2023 15:38:57 +0200 Subject: acpi: x86: remove _ADR on host bridges ACPI spec (since 2.0a) says " A device object must contain either an _HID object or an _ADR object, but can contain both. " _ADR is used when device is attached to an ennumerable bus, however hostbridge is not and uses dedicated _HID for discovery, drop _ADR field. It doesn't seem that having _ADR has a negative effects OSes manage to tolerate that, but there is no point of having it there. (only pc/q35 has it hostbridge description, while others (microvm/arm) don't) Signed-off-by: Igor Mammedov Message-Id: <20230720133858.1974024-6-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'hw') diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 19d268f..bb12b0a 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -1464,7 +1464,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, sb_scope = aml_scope("_SB"); dev = aml_device("PCI0"); aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); - aml_append(dev, aml_name_decl("_ADR", aml_int(0))); aml_append(dev, aml_name_decl("_UID", aml_int(pcmc->pci_root_uid))); aml_append(dev, aml_pci_edsm()); aml_append(sb_scope, dev); @@ -1479,7 +1478,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, dev = aml_device("PCI0"); aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); - aml_append(dev, aml_name_decl("_ADR", aml_int(0))); aml_append(dev, aml_name_decl("_UID", aml_int(pcmc->pci_root_uid))); aml_append(dev, build_q35_osc_method(!pm->pcihp_bridge_en)); aml_append(dev, aml_pci_edsm()); @@ -1593,7 +1591,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, aml_append(pkg, aml_eisaid("PNP0A08")); aml_append(pkg, aml_eisaid("PNP0A03")); aml_append(dev, aml_name_decl("_CID", pkg)); - aml_append(dev, aml_name_decl("_ADR", aml_int(0))); build_cxl_osc_method(dev); } else if (pci_bus_is_express(bus)) { aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); -- cgit v1.1 From 92f04221379ae5e35f6474c2afed2eb02d552df3 Mon Sep 17 00:00:00 2001 From: David Edmondson Date: Fri, 21 Jul 2023 08:28:20 +0100 Subject: hw/virtio: qmp: add RING_RESET to 'info virtio-status' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Edmondson Message-Id: <20230721072820.75797-1-david.edmondson@oracle.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Philippe Mathieu-Daudé --- hw/virtio/virtio-qmp.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'hw') diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c index 3d32dbe..7515b09 100644 --- a/hw/virtio/virtio-qmp.c +++ b/hw/virtio/virtio-qmp.c @@ -79,6 +79,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] = { "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"), FEATURE_ENTRY(VIRTIO_F_SR_IOV, \ "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"), + FEATURE_ENTRY(VIRTIO_F_RING_RESET, \ + "VIRTIO_F_RING_RESET: Driver can reset a queue individually"), /* Virtio ring transport features */ FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \ "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"), -- cgit v1.1 From c92f4fcafa14890524945073b494937e97112677 Mon Sep 17 00:00:00 2001 From: Hanna Czenczek Date: Fri, 21 Jul 2023 15:49:45 +0200 Subject: virtio: Fix packed virtqueue used_idx mask virtio_queue_packed_set_last_avail_idx() is used by vhost devices to set the internal queue indices to what has been reported by the vhost back-end through GET_VRING_BASE. For packed virtqueues, this 32-bit value is expected to contain both the device's internal avail and used indices, as well as their respective wrap counters. To get the used index, we shift the 32-bit value right by 16, and then apply a mask of 0x7ffff. That seems to be a typo, because it should be 0x7fff; first of all, the virtio specification says that the maximum queue size for packed virt queues is 2^15, so the indices cannot exceed 2^15 - 1 anyway, making 0x7fff the correct mask. Second, the mask clearly is wrong from context, too, given that (A) `idx & 0x70000` must be 0 at this point (`idx` is 32 bit and was shifted to the right by 16 already), (B) `idx & 0x8000` is the used_wrap_counter, so should not be part of the used index, and (C) `vq->used_idx` is a `uint16_t`, so cannot fit the 0x70000 part of the mask anyway. This most likely never produced any guest-visible bugs, though, because for a vhost device, qemu will probably not evaluate the used index outside of virtio_queue_packed_get_last_avail_idx(), where we reconstruct the 32-bit value from avail and used indices and their wrap counters again. There, it does not matter whether the highest bit of the used_idx is the used index wrap counter, because we put the wrap counter exactly in that position anyway. Signed-off-by: Hanna Czenczek Message-Id: <20230721134945.26967-1-hreitz@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: German Maglione --- hw/virtio/virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 295a603..309038f 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3321,7 +3321,7 @@ static void virtio_queue_packed_set_last_avail_idx(VirtIODevice *vdev, vq->last_avail_wrap_counter = vq->shadow_avail_wrap_counter = !!(idx & 0x8000); idx >>= 16; - vq->used_idx = idx & 0x7ffff; + vq->used_idx = idx & 0x7fff; vq->used_wrap_counter = !!(idx & 0x8000); } -- cgit v1.1 From 348e354417b64c484877354ee7cc66f29fa6c7df Mon Sep 17 00:00:00 2001 From: Yuri Benditovich Date: Fri, 28 Jul 2023 11:40:49 +0300 Subject: pci: do not respond config requests after PCI device eject Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2224964 In migration with VF failover, Windows guest and ACPI hot unplug we do not need to satisfy config requests, otherwise the guest immediately detects the device and brings up its driver. Many network VF's are stuck on the guest PCI bus after the migration. Signed-off-by: Yuri Benditovich Message-Id: <20230728084049.191454-1-yuri.benditovich@daynix.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci/pci_host.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c index 7af8afd..a18aa0a 100644 --- a/hw/pci/pci_host.c +++ b/hw/pci/pci_host.c @@ -62,6 +62,17 @@ static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit) } } +static bool is_pci_dev_ejected(PCIDevice *pci_dev) +{ + /* + * device unplug was requested and the guest acked it, + * so we stop responding config accesses even if the + * device is not deleted (failover flow) + */ + return pci_dev && pci_dev->partially_hotplugged && + !pci_dev->qdev.pending_deleted_event; +} + void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, uint32_t limit, uint32_t val, uint32_t len) { @@ -75,7 +86,7 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, * allowing direct removal of unexposed functions. */ if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) || - !pci_dev->has_power) { + !pci_dev->has_power || is_pci_dev_ejected(pci_dev)) { return; } @@ -100,7 +111,7 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, * allowing direct removal of unexposed functions. */ if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) || - !pci_dev->has_power) { + !pci_dev->has_power || is_pci_dev_ejected(pci_dev)) { return ~0x0; } -- cgit v1.1 From 18f2971ce403008d5e1c2875b483c9d1778143dc Mon Sep 17 00:00:00 2001 From: Li Feng Date: Mon, 31 Jul 2023 20:10:06 +0800 Subject: vhost: fix the fd leak When the vhost-user reconnect to the backend, the notifer should be cleanup. Otherwise, the fd resource will be exhausted. Fixes: f9a09ca3ea ("vhost: add support for configure interrupt") Signed-off-by: Li Feng Reviewed-by: Raphael Norwitz Message-Id: <20230731121018.2856310-2-fengli@smartx.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Tested-by: Fiona Ebner --- hw/virtio/vhost.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'hw') diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index abf0d03..e2f6ffb 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -2044,6 +2044,8 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) event_notifier_test_and_clear( &hdev->vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier); event_notifier_test_and_clear(&vdev->config_notifier); + event_notifier_cleanup( + &hdev->vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier); trace_vhost_dev_stop(hdev, vdev->name, vrings); -- cgit v1.1 From cc2a08480e19007c05be8fe5b6893e20448954dc Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 2 Aug 2023 15:57:18 +0200 Subject: hw/i386/intel_iommu: Fix trivial endianness problems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After reading the guest memory with dma_memory_read(), we have to make sure that we byteswap the little endian data to the host's byte order. Signed-off-by: Thomas Huth Message-Id: <20230802135723.178083-2-thuth@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Peter Xu --- hw/i386/intel_iommu.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'hw') diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index dcc3340..13fcde8 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -756,6 +756,8 @@ static int vtd_get_pdire_from_pdir_table(dma_addr_t pasid_dir_base, return -VTD_FR_PASID_TABLE_INV; } + pdire->val = le64_to_cpu(pdire->val); + return 0; } @@ -780,6 +782,9 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s, pe, entry_size, MEMTXATTRS_UNSPECIFIED)) { return -VTD_FR_PASID_TABLE_INV; } + for (size_t i = 0; i < ARRAY_SIZE(pe->val); i++) { + pe->val[i] = le64_to_cpu(pe->val[i]); + } /* Do translation type check */ if (!vtd_pe_type_check(x86_iommu, pe)) { -- cgit v1.1 From 642ba89672279fbdd14016a90da239c85e845d18 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 2 Aug 2023 15:57:19 +0200 Subject: hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry The code already tries to do some endianness handling here, but currently fails badly: - While it already swaps the data when logging errors / tracing, it fails to byteswap the value before e.g. accessing entry->irte.present - entry->irte.source_id is swapped with le32_to_cpu(), though this is a 16-bit value - The whole union is apparently supposed to be swapped via the 64-bit data[2] array, but the struct is a mixture between 32 bit values (the first 8 bytes) and 64 bit values (the second 8 bytes), so this cannot work as expected. Fix it by converting the struct to two proper 64-bit bitfields, and by swapping the values only once for everybody right after reading the data from memory. Signed-off-by: Thomas Huth Message-Id: <20230802135723.178083-3-thuth@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Peter Xu --- hw/i386/intel_iommu.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'hw') diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 13fcde8..4028e32 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3328,14 +3328,15 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index, return -VTD_FR_IR_ROOT_INVAL; } - trace_vtd_ir_irte_get(index, le64_to_cpu(entry->data[1]), - le64_to_cpu(entry->data[0])); + entry->data[0] = le64_to_cpu(entry->data[0]); + entry->data[1] = le64_to_cpu(entry->data[1]); + + trace_vtd_ir_irte_get(index, entry->data[1], entry->data[0]); if (!entry->irte.present) { error_report_once("%s: detected non-present IRTE " "(index=%u, high=0x%" PRIx64 ", low=0x%" PRIx64 ")", - __func__, index, le64_to_cpu(entry->data[1]), - le64_to_cpu(entry->data[0])); + __func__, index, entry->data[1], entry->data[0]); return -VTD_FR_IR_ENTRY_P; } @@ -3343,14 +3344,13 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index, entry->irte.__reserved_2) { error_report_once("%s: detected non-zero reserved IRTE " "(index=%u, high=0x%" PRIx64 ", low=0x%" PRIx64 ")", - __func__, index, le64_to_cpu(entry->data[1]), - le64_to_cpu(entry->data[0])); + __func__, index, entry->data[1], entry->data[0]); return -VTD_FR_IR_IRTE_RSVD; } if (sid != X86_IOMMU_SID_INVALID) { /* Validate IRTE SID */ - source_id = le32_to_cpu(entry->irte.source_id); + source_id = entry->irte.source_id; switch (entry->irte.sid_vtype) { case VTD_SVT_NONE: break; @@ -3404,7 +3404,7 @@ static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, irq->trigger_mode = irte.irte.trigger_mode; irq->vector = irte.irte.vector; irq->delivery_mode = irte.irte.delivery_mode; - irq->dest = le32_to_cpu(irte.irte.dest_id); + irq->dest = irte.irte.dest_id; if (!iommu->intr_eime) { #define VTD_IR_APIC_DEST_MASK (0xff00ULL) #define VTD_IR_APIC_DEST_SHIFT (8) -- cgit v1.1 From 4572b22cf9ba432fa3955686853c706a1821bbc7 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 2 Aug 2023 15:57:20 +0200 Subject: hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts On big endian hosts, we need to reverse the bitfield order in the struct VTDInvDescIEC, just like it is already done for the other bitfields in the various structs of the intel-iommu device. Signed-off-by: Thomas Huth Message-Id: <20230802135723.178083-4-thuth@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Peter Xu --- hw/i386/intel_iommu_internal.h | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'hw') diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h index 2e61eec..e1450c5 100644 --- a/hw/i386/intel_iommu_internal.h +++ b/hw/i386/intel_iommu_internal.h @@ -321,12 +321,21 @@ typedef enum VTDFaultReason { /* Interrupt Entry Cache Invalidation Descriptor: VT-d 6.5.2.7. */ struct VTDInvDescIEC { +#if HOST_BIG_ENDIAN + uint64_t reserved_2:16; + uint64_t index:16; /* Start index to invalidate */ + uint64_t index_mask:5; /* 2^N for continuous int invalidation */ + uint64_t resved_1:22; + uint64_t granularity:1; /* If set, it's global IR invalidation */ + uint64_t type:4; /* Should always be 0x4 */ +#else uint32_t type:4; /* Should always be 0x4 */ uint32_t granularity:1; /* If set, it's global IR invalidation */ uint32_t resved_1:22; uint32_t index_mask:5; /* 2^N for continuous int invalidation */ uint32_t index:16; /* Start index to invalidate */ uint32_t reserved_2:16; +#endif }; typedef struct VTDInvDescIEC VTDInvDescIEC; -- cgit v1.1 From fcd8027423300b201b37842b88393dc5c6c8ee9e Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 2 Aug 2023 15:57:21 +0200 Subject: hw/i386/intel_iommu: Fix index calculation in vtd_interrupt_remap_msi() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The values in "addr" are populated locally in this function in host endian byte order, so we must not swap the index_l field here. Signed-off-by: Thomas Huth Message-Id: <20230802135723.178083-5-thuth@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Peter Xu --- hw/i386/intel_iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 4028e32..3ca71df 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3459,7 +3459,7 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu, goto out; } - index = addr.addr.index_h << 15 | le16_to_cpu(addr.addr.index_l); + index = addr.addr.index_h << 15 | addr.addr.index_l; #define VTD_IR_MSI_DATA_SUBHANDLE (0x0000ffff) #define VTD_IR_MSI_DATA_RESERVED (0xffff0000) -- cgit v1.1 From 37cf5cecb039a063c0abe3b51ae30f969e73aa84 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 2 Aug 2023 15:57:22 +0200 Subject: hw/i386/x86-iommu: Fix endianness issue in x86_iommu_irq_to_msi_message() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The values in "msg" are assembled in host endian byte order (the other field are also not swapped), so we must not swap the __addr_head here. Signed-off-by: Thomas Huth Message-Id: <20230802135723.178083-6-thuth@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Peter Xu --- hw/i386/x86-iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c index 01d1132..726e9e1 100644 --- a/hw/i386/x86-iommu.c +++ b/hw/i386/x86-iommu.c @@ -63,7 +63,7 @@ void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq, MSIMessage *msg_out) msg.redir_hint = irq->redir_hint; msg.dest = irq->dest; msg.__addr_hi = irq->dest & 0xffffff00; - msg.__addr_head = cpu_to_le32(0xfee); + msg.__addr_head = 0xfee; /* Keep this from original MSI address bits */ msg.__not_used = irq->msi_addr_last_bits; -- cgit v1.1 From 9d38a8434721a6479fe03fb5afb150ca793d3980 Mon Sep 17 00:00:00 2001 From: zhenwei pi Date: Thu, 3 Aug 2023 10:43:13 +0800 Subject: virtio-crypto: verify src&dst buffer length for sym request For symmetric algorithms, the length of ciphertext must be as same as the plaintext. The missing verification of the src_len and the dst_len in virtio_crypto_sym_op_helper() may lead buffer overflow/divulged. This patch is originally written by Yiming Tao for QEMU-SECURITY, resend it(a few changes of error message) in qemu-devel. Fixes: CVE-2023-3180 Fixes: 04b9b37edda("virtio-crypto: add data queue processing handler") Cc: Gonglei Cc: Mauro Matteo Cascella Cc: Yiming Tao Signed-off-by: zhenwei pi Message-Id: <20230803024314.29962-2-pizhenwei@bytedance.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio-crypto.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'hw') diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c index 44faf5a..13aec77 100644 --- a/hw/virtio/virtio-crypto.c +++ b/hw/virtio/virtio-crypto.c @@ -634,6 +634,11 @@ virtio_crypto_sym_op_helper(VirtIODevice *vdev, return NULL; } + if (unlikely(src_len != dst_len)) { + virtio_error(vdev, "sym request src len is different from dst len"); + return NULL; + } + max_len = (uint64_t)iv_len + aad_len + src_len + dst_len + hash_result_len; if (unlikely(max_len > vcrypto->conf.max_size)) { virtio_error(vdev, "virtio-crypto too big length"); -- cgit v1.1