From nobody Fri Nov 7 07:34:23 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1546962971399405.4486685318544; Tue, 8 Jan 2019 07:56:11 -0800 (PST) Received: from localhost ([127.0.0.1]:51058 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ggtje-0005Kb-AZ for importer@patchew.org; Tue, 08 Jan 2019 10:56:10 -0500 Received: from eggs.gnu.org ([209.51.188.92]:37807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ggthd-0003rp-E9 for qemu-devel@nongnu.org; Tue, 08 Jan 2019 10:54:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ggthc-0006a8-Aw for qemu-devel@nongnu.org; Tue, 08 Jan 2019 10:54:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59666) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ggthb-0006ST-FC for qemu-devel@nongnu.org; Tue, 08 Jan 2019 10:54:04 -0500 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A5B88CA38C; Tue, 8 Jan 2019 15:54:00 +0000 (UTC) Received: from sirius.home.kraxel.org (ovpn-116-98.ams2.redhat.com [10.36.116.98]) by smtp.corp.redhat.com (Postfix) with ESMTP id B336F620D9; Tue, 8 Jan 2019 15:53:55 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id F01D2981A; Tue, 8 Jan 2019 16:53:54 +0100 (CET) From: Gerd Hoffmann To: qemu-devel@nongnu.org Date: Tue, 8 Jan 2019 16:53:51 +0100 Message-Id: <20190108155354.8591-3-kraxel@redhat.com> In-Reply-To: <20190108155354.8591-1-kraxel@redhat.com> References: <20190108155354.8591-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 08 Jan 2019 15:54:00 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL 2/5] usb: drop unnecessary usb_device_post_load checks X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Eduardo Habkost , "Michael S. Tsirkin" , Gerd Hoffmann , Jonathan Davies , Paolo Bonzini , Richard Henderson Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" From: Jonathan Davies 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=3Dy and HID_MAX_BUFFER_SIZE > 4096: #include #include #include #include int main() { char buf[4097]; int fd =3D open("/dev/hidraw0", O_RDWR|O_NONBLOCK); buf[0] =3D 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:0= 6.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 Message-Id: <20190107175117.23769-1-jonathan.davies@nutanix.com> Signed-off-by: Gerd Hoffmann --- hw/usb/bus.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hw/usb/bus.c b/hw/usb/bus.c index bf796d67e6..6fffab7bfa 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -59,12 +59,6 @@ static int usb_device_post_load(void *opaque, int versio= n_id) } else { dev->attached =3D 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; } =20 --=20 2.9.3