diff mbox series

[v2] virtio: skip guest index check on device load

Message ID 20201028134643.110698-1-felipe@nutanix.com (mailing list archive)
State New, archived
Headers show
Series [v2] virtio: skip guest index check on device load | expand

Commit Message

Felipe Franciosi Oct. 28, 2020, 1:47 p.m. UTC
QEMU must be careful when loading device state off migration streams to
prevent a malicious source from exploiting the emulator. Overdoing these
checks has the side effect of allowing a guest to "pin itself" in cloud
environments by messing with state which is entirely in its control.

Similarly to what f3081539 achieved in usb_device_post_load(), this
commit removes such a check from virtio_load(). Worth noting, the result
of a load without this check is the same as if a guest enables a VQ with
invalid indexes to begin with. That is, the virtual device is set in a
broken state (by the datapath handler) and must be reset.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
v2: Keep the check around for logging.

 hw/virtio/virtio.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Jason Wang Oct. 29, 2020, 7:02 a.m. UTC | #1
On 2020/10/28 下午9:47, Felipe Franciosi wrote:
> QEMU must be careful when loading device state off migration streams to
> prevent a malicious source from exploiting the emulator. Overdoing these
> checks has the side effect of allowing a guest to "pin itself" in cloud
> environments by messing with state which is entirely in its control.
>
> Similarly to what f3081539 achieved in usb_device_post_load(), this
> commit removes such a check from virtio_load(). Worth noting, the result
> of a load without this check is the same as if a guest enables a VQ with
> invalid indexes to begin with. That is, the virtual device is set in a
> broken state (by the datapath handler) and must be reset.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
> v2: Keep the check around for logging.
>
>   hw/virtio/virtio.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 6f8f865aff..ceb58fda6c 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -17,6 +17,7 @@
>   #include "trace.h"
>   #include "exec/address-spaces.h"
>   #include "qemu/error-report.h"
> +#include "qemu/log.h"
>   #include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   #include "hw/virtio/virtio.h"
> @@ -3160,12 +3161,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>               nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
>               /* Check it isn't doing strange things with descriptor numbers. */
>               if (nheads > vdev->vq[i].vring.num) {
> -                error_report("VQ %d size 0x%x Guest index 0x%x "
> -                             "inconsistent with Host index 0x%x: delta 0x%x",
> -                             i, vdev->vq[i].vring.num,
> -                             vring_avail_idx(&vdev->vq[i]),
> -                             vdev->vq[i].last_avail_idx, nheads);
> -                return -1;
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "VQ %d size 0x%x Guest index 0x%x "
> +                              "inconsistent with Host index 0x%x: delta 0x%x",
> +                              i, vdev->vq[i].vring.num,
> +                              vring_avail_idx(&vdev->vq[i]),
> +                              vdev->vq[i].last_avail_idx, nheads);
>               }
>               vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
>               vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);


Acked-by: Jason Wang <jasowang@redhat.com>
diff mbox series

Patch

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6f8f865aff..ceb58fda6c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -17,6 +17,7 @@ 
 #include "trace.h"
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
+#include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "hw/virtio/virtio.h"
@@ -3160,12 +3161,12 @@  int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
             /* Check it isn't doing strange things with descriptor numbers. */
             if (nheads > vdev->vq[i].vring.num) {
-                error_report("VQ %d size 0x%x Guest index 0x%x "
-                             "inconsistent with Host index 0x%x: delta 0x%x",
-                             i, vdev->vq[i].vring.num,
-                             vring_avail_idx(&vdev->vq[i]),
-                             vdev->vq[i].last_avail_idx, nheads);
-                return -1;
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "VQ %d size 0x%x Guest index 0x%x "
+                              "inconsistent with Host index 0x%x: delta 0x%x",
+                              i, vdev->vq[i].vring.num,
+                              vring_avail_idx(&vdev->vq[i]),
+                              vdev->vq[i].last_avail_idx, nheads);
             }
             vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
             vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);