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