Pablo Greco e6a3ae
From 24ca1010222cadbfc3c734406b665e6a9775d9d9 Mon Sep 17 00:00:00 2001
Pablo Greco e6a3ae
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Pablo Greco e6a3ae
Date: Tue, 1 Oct 2019 18:49:25 +0100
Pablo Greco e6a3ae
Subject: [PATCH 03/21] usb: drop unnecessary usb_device_post_load checks
Pablo Greco e6a3ae
Pablo Greco e6a3ae
RH-Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
Pablo Greco e6a3ae
Message-id: <20191001184925.29912-2-dgilbert@redhat.com>
Pablo Greco e6a3ae
Patchwork-id: 90933
Pablo Greco e6a3ae
O-Subject: [RHEL-8.2.0 qemu-kvm PATCH 1/1] usb: drop unnecessary usb_device_post_load checks
Pablo Greco e6a3ae
Bugzilla: 1757482
Pablo Greco e6a3ae
RH-Acked-by: Maxim Levitsky <mlevitsk@redhat.com>
Pablo Greco e6a3ae
RH-Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Pablo Greco e6a3ae
RH-Acked-by: Igor Mammedov <imammedo@redhat.com>
Pablo Greco e6a3ae
Pablo Greco e6a3ae
From: Jonathan Davies <jonathan.davies@nutanix.com>
Pablo Greco e6a3ae
Pablo Greco e6a3ae
In usb_device_post_load, certain values of dev->setup_len or
Pablo Greco e6a3ae
dev->setup_index can cause -EINVAL to be returned. One example is when
Pablo Greco e6a3ae
setup_len exceeds 4096, the hard-coded value of sizeof(dev->data_buf).
Pablo Greco e6a3ae
This can happen through legitimate guest activity and will cause all
Pablo Greco e6a3ae
subsequent attempts to migrate the guest to fail in vmstate_load_state.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
The values of these variables can be set by USB packets originating in
Pablo Greco e6a3ae
the guest. There are two ways in which they can be set: in
Pablo Greco e6a3ae
do_token_setup and in do_parameter in hw/usb/core.c.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
It is easy to craft a USB packet in a guest that causes do_token_setup
Pablo Greco e6a3ae
to set setup_len to a value larger than 4096. When this has been done
Pablo Greco e6a3ae
once, all subsequent attempts to migrate the VM will fail in
Pablo Greco e6a3ae
usb_device_post_load until the VM is next power-cycled or a
Pablo Greco e6a3ae
smaller-sized USB packet is sent to the device.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
Sample code for achieving this in a VM started with "-device usb-tablet"
Pablo Greco e6a3ae
running Linux with CONFIG_HIDRAW=y and HID_MAX_BUFFER_SIZE > 4096:
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  #include <sys types.h="">
Pablo Greco e6a3ae
  #include <sys stat.h="">
Pablo Greco e6a3ae
  #include <fcntl.h>
Pablo Greco e6a3ae
  #include <unistd.h>
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  int main() {
Pablo Greco e6a3ae
           char buf[4097];
Pablo Greco e6a3ae
           int fd = open("/dev/hidraw0", O_RDWR|O_NONBLOCK);
Pablo Greco e6a3ae
Pablo Greco e6a3ae
           buf[0] = 0x1;
Pablo Greco e6a3ae
           write(fd, buf, 4097);
Pablo Greco e6a3ae
Pablo Greco e6a3ae
           return 0;
Pablo Greco e6a3ae
  }
Pablo Greco e6a3ae
Pablo Greco e6a3ae
When this code is run in the VM, qemu will output:
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  usb_generic_handle_packet: ctrl buffer too small (4097 > 4096)
Pablo Greco e6a3ae
Pablo Greco e6a3ae
A subsequent attempt to migrate the VM will fail and output the
Pablo Greco e6a3ae
following on the destination host:
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  qemu-kvm: error while loading state for instance 0x0 of device '0000:00:06.7/1/usb-ptr'
Pablo Greco e6a3ae
  qemu-kvm: load of migration failed: Invalid argument
Pablo Greco e6a3ae
Pablo Greco e6a3ae
The idea behind checking the values of setup_len and setup_index before
Pablo Greco e6a3ae
they are used is correct, but doing it in usb_device_post_load feels
Pablo Greco e6a3ae
arbitrary, and will cause unnecessary migration failures. Indeed, none
Pablo Greco e6a3ae
of the commit messages for c60174e8, 9f8e9895 and 719ffe1f justify why
Pablo Greco e6a3ae
post_load is the right place to do these checks. They correctly point
Pablo Greco e6a3ae
out that the important thing to protect is the usb_packet_copy.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
Instead, the right place to do the checks is in do_token_setup and
Pablo Greco e6a3ae
do_parameter. Indeed, there are already some checks here. We can examine
Pablo Greco e6a3ae
each of the disjuncts currently tested in usb_device_post_load to see
Pablo Greco e6a3ae
whether any need adding to do_token_setup or do_parameter to improve
Pablo Greco e6a3ae
safety there:
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  * dev->setup_index < 0
Pablo Greco e6a3ae
     - This test is not needed because setup_index is explicitly set to
Pablo Greco e6a3ae
0 in do_token_setup and do_parameter.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  * dev->setup_len < 0
Pablo Greco e6a3ae
     - In both do_token_setup and do_parameter, the value of setup_len
Pablo Greco e6a3ae
is computed by (s->setup_buf[7] << 8) | s->setup_buf[6]. Since
Pablo Greco e6a3ae
s->setup_buf is a byte array and setup_len is an int32_t, it's
Pablo Greco e6a3ae
impossible for this arithmetic to set setup_len's top bit, so it can
Pablo Greco e6a3ae
never be negative.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  * dev->setup_index > dev->setup_len
Pablo Greco e6a3ae
     - Since setup_index is 0, this is equivalent to the previous test,
Pablo Greco e6a3ae
so is redundant.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
  * dev->setup_len > sizeof(dev->data_buf)
Pablo Greco e6a3ae
     - This condition is already explicitly checked in both
Pablo Greco e6a3ae
do_token_setup and do_parameter.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
Hence there is no need to bolster the existing checks in do_token_setup
Pablo Greco e6a3ae
or do_parameter, and we can safely remove these checks from
Pablo Greco e6a3ae
usb_device_post_load without reducing safety but allowing migrations to
Pablo Greco e6a3ae
proceed regardless of what USB packets have been generated by the guest.
Pablo Greco e6a3ae
Pablo Greco e6a3ae
Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com>
Pablo Greco e6a3ae
Message-Id: <20190107175117.23769-1-jonathan.davies@nutanix.com>
Pablo Greco e6a3ae
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Pablo Greco e6a3ae
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Pablo Greco e6a3ae
(cherry picked from commit f30815390adb1ec153327c3832ab378e8bce9808)
Pablo Greco e6a3ae
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
Pablo Greco e6a3ae
---
Pablo Greco e6a3ae
 hw/usb/bus.c | 6 ------
Pablo Greco e6a3ae
 1 file changed, 6 deletions(-)
Pablo Greco e6a3ae
Pablo Greco e6a3ae
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
Pablo Greco e6a3ae
index 11f7720..5499810 100644
Pablo Greco e6a3ae
--- a/hw/usb/bus.c
Pablo Greco e6a3ae
+++ b/hw/usb/bus.c
Pablo Greco e6a3ae
@@ -59,12 +59,6 @@ static int usb_device_post_load(void *opaque, int version_id)
Pablo Greco e6a3ae
     } else {
Pablo Greco e6a3ae
         dev->attached = true;
Pablo Greco e6a3ae
     }
Pablo Greco e6a3ae
-    if (dev->setup_index < 0 ||
Pablo Greco e6a3ae
-        dev->setup_len < 0 ||
Pablo Greco e6a3ae
-        dev->setup_index > dev->setup_len ||
Pablo Greco e6a3ae
-        dev->setup_len > sizeof(dev->data_buf)) {
Pablo Greco e6a3ae
-        return -EINVAL;
Pablo Greco e6a3ae
-    }
Pablo Greco e6a3ae
     return 0;
Pablo Greco e6a3ae
 }
Pablo Greco e6a3ae
 
Pablo Greco e6a3ae
-- 
Pablo Greco e6a3ae
1.8.3.1
Pablo Greco e6a3ae