[PULL 0/3] virtio,pc: bugfixes

[PULL 0/3] virtio,pc: bugfixes

[Michael S. Tsirkin]

The following changes since commit 128e050d41794e61e5849c6c507160da5556ea61:

  hw/acpi/microvm: turn on 8042 bit in FADT boot architecture flags if present (2022-03-07 17:43:14 -0500)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to f556b9a0cd20d41493afd403fb7f016c8fb01eb3:

  virtio-iommu: use-after-free fix (2022-04-06 17:11:03 -0400)

----------------------------------------------------------------
virtio,pc: bugfixes

Several fixes all over the place

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Halil Pasic (1):
      virtio: fix feature negotiation for ACCESS_PLATFORM

Jason Wang (1):
      intel-iommu: correct the value used for error_setg_errno()

Wentao Liang (1):
      virtio-iommu: use-after-free fix

 hw/i386/intel_iommu.c    |  2 +-
 hw/virtio/virtio-bus.c   | 22 ++++++++++++++--------
 hw/virtio/virtio-iommu.c |  1 +
 3 files changed, 16 insertions(+), 9 deletions(-)

[PULL 1/3] virtio: fix feature negotiation for ACCESS_PLATFORM

[Michael S. Tsirkin]

From: Halil Pasic <pasic@linux.ibm.com>

Unlike most virtio features ACCESS_PLATFORM is considered mandatory by
QEMU, i.e. the driver must accept it if offered by the device. The
virtio specification says that the driver SHOULD accept the
ACCESS_PLATFORM feature if offered, and that the device MAY fail to
operate if ACCESS_PLATFORM was offered but not negotiated.

While a SHOULD ain't exactly a MUST, we are certainly allowed to fail
the device when the driver fences ACCESS_PLATFORM. With commit
2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") we already made the
decision to do so whenever the get_dma_as() callback is implemented (by
the bus), which in practice means for the entirety of virtio-pci.

That means, if the device needs to translate I/O addresses, then
ACCESS_PLATFORM is mandatory. The aforementioned commit tells us in the
commit message that this is for security reasons. More precisely if we
were to allow a less then trusted driver (e.g. an user-space driver, or
a nested guest) to make the device bypass the IOMMU by not negotiating
ACCESS_PLATFORM, then the guest kernel would have no ability to
control/police (by programming the IOMMU) what pieces of guest memory
the driver may manipulate using the device. Which would break security
assumptions within the guest.

If ACCESS_PLATFORM is offered not because we want the device to utilize
an IOMMU and do address translation, but because the device does not
have access to the entire guest RAM, and needs the driver to grant
access to the bits it needs access to (e.g. confidential guest support),
we still require the guest to have the corresponding logic and to accept
ACCESS_PLATFORM. If the driver does not accept ACCESS_PLATFORM, then
things are bound to go wrong, and we may see failures much less graceful
than failing the device because the driver didn't negotiate
ACCESS_PLATFORM.

So let us make ACCESS_PLATFORM mandatory for the driver regardless
of whether the get_dma_as() callback is implemented or not.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM")

Message-Id: <20220307112939.2780117-1-pasic@linux.ibm.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/virtio/virtio-bus.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 0f69d1c742..d7ec023adf 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -78,17 +78,23 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
         return;
     }
 
-    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
-    if (klass->get_dma_as != NULL && has_iommu) {
+    vdev->dma_as = &address_space_memory;
+    if (has_iommu) {
+        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+        /*
+         * Present IOMMU_PLATFORM to the driver iff iommu_plattform=on and
+         * device operational. If the driver does not accept IOMMU_PLATFORM
+         * we fail the device.
+         */
         virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
-        vdev->dma_as = klass->get_dma_as(qbus->parent);
-        if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
-            error_setg(errp,
+        if (klass->get_dma_as) {
+            vdev->dma_as = klass->get_dma_as(qbus->parent);
+            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
+                error_setg(errp,
                        "iommu_platform=true is not supported by the device");
-            return;
+                return;
+            }
         }
-    } else {
-        vdev->dma_as = &address_space_memory;
     }
 }
 
-- 
MST

[PULL 2/3] intel-iommu: correct the value used for error_setg_errno()

[Michael S. Tsirkin]

From: Jason Wang <jasowang@redhat.com>

error_setg_errno() expects a normal errno value, not a negated
one, so we should use ENOTSUP instead of -ENOSUP.

Fixes: Coverity CID 1487174
Fixes: ("intel_iommu: support snoop control")
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20220401022824.9337-1-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 32471a44cb..b4b4c82be6 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3032,7 +3032,7 @@ static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
 
     /* TODO: add support for VFIO and vhost users */
     if (s->snoop_control) {
-        error_setg_errno(errp, -ENOTSUP,
+        error_setg_errno(errp, ENOTSUP,
                          "Snoop Control with vhost or VFIO is not supported");
         return -ENOTSUP;
     }
-- 
MST

[PULL 3/3] virtio-iommu: use-after-free fix

[Michael S. Tsirkin]

From: Wentao Liang <Wentao_Liang_g@163.com>

A potential Use-after-free was reported in virtio_iommu_handle_command
when using virtio-iommu:

> I find a potential Use-after-free in QEMU 6.2.0, which is in
> virtio_iommu_handle_command() (./hw/virtio/virtio-iommu.c).
>
>
> Specifically, in the loop body, the variable 'buf' allocated at line 639 can be
> freed by g_free() at line 659. However, if the execution path enters the loop
> body again and the if branch takes true at line 616, the control will directly
> jump to 'out' at line 651. At this time, 'buf' is a freed pointer, which is not
> assigned with an allocated memory but used at line 653. As a result, a UAF bug
> is triggered.
>
>
>
> 599     for (;;) {
> ...
> 615         sz = iov_to_buf(iov, iov_cnt, 0, &head, sizeof(head));
> 616         if (unlikely(sz != sizeof(head))) {
> 617             tail.status = VIRTIO_IOMMU_S_DEVERR;
> 618             goto out;
> 619         }
> ...
> 639             buf = g_malloc0(output_size);
> ...
> 651 out:
> 652         sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> 653                           buf ? buf : &tail, output_size);
> ...
> 659         g_free(buf);
>
> We can fix it by set ‘buf‘ to NULL after freeing it:
>
>
> 651 out:
> 652         sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> 653                           buf ? buf : &tail, output_size);
> ...
> 659         g_free(buf);
> +++ buf = NULL;
> 660     }

Fix as suggested by the reporter.

Signed-off-by: Wentao Liang <Wentao_Liang_g@163.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Message-ID: <20220406040445-mutt-send-email-mst@kernel.org>
---
 hw/virtio/virtio-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 239fe97b12..2b1d21edd1 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -683,6 +683,7 @@ out:
         virtio_notify(vdev, vq);
         g_free(elem);
         g_free(buf);
+        buf = NULL;
     }
 }
 
-- 
MST

Re: [PULL 0/3] virtio,pc: bugfixes

[Peter Maydell]

On Wed, 6 Apr 2022 at 22:11, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit 128e050d41794e61e5849c6c507160da5556ea61:
>
>   hw/acpi/microvm: turn on 8042 bit in FADT boot architecture flags if present (2022-03-07 17:43:14 -0500)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to f556b9a0cd20d41493afd403fb7f016c8fb01eb3:
>
>   virtio-iommu: use-after-free fix (2022-04-06 17:11:03 -0400)
>
> ----------------------------------------------------------------
> virtio,pc: bugfixes
>
> Several fixes all over the place
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

RC3 has gone. We're not taking any more changes unless they
are absolutely release-critical, which means that you need
to be clearly describing in the pull request cover letter
what the changes are and why they are release critical.

thanks
-- PMM

Re: [PULL 0/3] virtio,pc: bugfixes

[Michael S. Tsirkin]

On Thu, Apr 07, 2022 at 10:18:24AM +0100, Peter Maydell wrote:
> On Wed, 6 Apr 2022 at 22:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > The following changes since commit 128e050d41794e61e5849c6c507160da5556ea61:
> >
> >   hw/acpi/microvm: turn on 8042 bit in FADT boot architecture flags if present (2022-03-07 17:43:14 -0500)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to f556b9a0cd20d41493afd403fb7f016c8fb01eb3:
> >
> >   virtio-iommu: use-after-free fix (2022-04-06 17:11:03 -0400)
> >
> > ----------------------------------------------------------------
> > virtio,pc: bugfixes
> >
> > Several fixes all over the place
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> RC3 has gone. We're not taking any more changes unless they
> are absolutely release-critical, which means that you need
> to be clearly describing in the pull request cover letter
> what the changes are and why they are release critical.
> 
> thanks
> -- PMM

Will do, thanks!

-- 
MST

Re: [PULL 0/3] virtio,pc: bugfixes

[Michael S. Tsirkin]

On Thu, Apr 07, 2022 at 10:18:24AM +0100, Peter Maydell wrote:
> On Wed, 6 Apr 2022 at 22:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > The following changes since commit 128e050d41794e61e5849c6c507160da5556ea61:
> >
> >   hw/acpi/microvm: turn on 8042 bit in FADT boot architecture flags if present (2022-03-07 17:43:14 -0500)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to f556b9a0cd20d41493afd403fb7f016c8fb01eb3:
> >
> >   virtio-iommu: use-after-free fix (2022-04-06 17:11:03 -0400)
> >
> > ----------------------------------------------------------------
> > virtio,pc: bugfixes
> >
> > Several fixes all over the place
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> RC3 has gone. We're not taking any more changes unless they
> are absolutely release-critical, which means that you need
> to be clearly describing in the pull request cover letter
> what the changes are and why they are release critical.
> 
> thanks
> -- PMM

You know, after looking at it critically ;) I just sent the release critical
patch on list. Feel free to pick it up.

-- 
MST