usb: drop unnecessary usb_device_post_load checks
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 forc60174e8
,9f8e9895
and719ffe1f
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>
This commit is contained in:
parent
3fd2092fd1
commit
f30815390a
|
@ -59,12 +59,6 @@ static int usb_device_post_load(void *opaque, int version_id)
|
||||||
} else {
|
} else {
|
||||||
dev->attached = true;
|
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;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue