[PATCH] usb: gadget: f_printer: prevent OOB write in GET_DEVICE_ID

[Haofeng Li]

printer_func_setup() services the Printer Class GET_DEVICE_ID request
by echoing the PnP string previously stored in the gadget's configfs
pnp_string attribute:

	value = strlen(*dev->pnp_string);
	buf[0] = (value >> 8) & 0xFF;
	buf[1] = value & 0xFF;
	memcpy(buf + 2, *dev->pnp_string, value);

The EP0 response buffer is exactly USB_COMP_EP0_BUFSIZ (4096) bytes,
allocated once by composite_dev_prepare():

	cdev->req->buf = kzalloc(USB_COMP_EP0_BUFSIZ, GFP_KERNEL);

The two-byte length prefix plus the string body must therefore fit
into 4096 bytes.  pnp_string is stored via kstrndup(page, len,
GFP_KERNEL) in f_printer_opts_pnp_string_store(); configfs passes at
most PAGE_SIZE - 1 (4095) bytes down to the store callback, so a
4095-byte string combined with the 2-byte length field makes the
memcpy() write buf[2..4096], one byte past the end of the allocation.

Attack chain (USB Printer gadget on the victim device):

  1. pnp_string is set to a 4095-byte value through the gadget's
     configfs attribute
     (~/config/usb_gadget/<gadget>/functions/printer.usb0/pnp_string);
     configfs accepts up to PAGE_SIZE - 1 bytes (fs/configfs/file.c).
  2. The printer function is enabled and the gadget is bound to its
     UDC.  An attacker in control of the connecting USB host sends a
     Printer Class GET_DEVICE_ID request (bmRequestType=0xA1,
     bRequest=0x00, wIndex pointing at the printer interface); the
     usblp host driver also issues this request on enumeration.
  3. composite_setup() -> printer_func_setup() -> memcpy(buf + 2,
     pnp_string, 4095) performs a 4097-byte write into the 4096-byte
     EP0 response buffer, overflowing the heap object by one byte and
     potentially corrupting adjacent slab objects or allocator
     metadata (CWE-787).

With KASAN enabled the overflow is reliably reported (this is
reproducible end to end with a configfs gadget + dummy_hcd):

	BUG: KASAN: slab-out-of-bounds in printer_func_setup+0x2ec/0x3c0
	Write of size 4095 at addr ffff88818e461002

Fix it at both ends:

  - clamp the string length to USB_COMP_EP0_BUFSIZ - 2 in
    printer_func_setup() so the copy can never exceed the EP0 buffer,
    and
  - reject pnp_string values longer than USB_COMP_EP0_BUFSIZ - 2 in
    f_printer_opts_pnp_string_store() so an oversized string is never
    stored in the first place.

Signed-off-by: Haofeng Li <lihaofeng@xxxxxxxxxx>
---
 drivers/usb/gadget/function/f_printer.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 1857d786110b..4b28e73d35ca 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -1035,6 +1035,17 @@ static int printer_func_setup(struct usb_function *f,
 				break;
 			}
 			value = strlen(*dev->pnp_string);
+			/*
+			 * The EP0 response buffer is USB_COMP_EP0_BUFSIZ
+			 * bytes and the first two bytes hold the string
+			 * length, so at most USB_COMP_EP0_BUFSIZ - 2 bytes
+			 * of the PnP string can be copied.  A string stored
+			 * through configfs is at most USB_COMP_EP0_BUFSIZ - 1
+			 * bytes long, which would overflow the buffer by one
+			 * byte here, so clamp it before the memcpy() below.
+			 */
+			if (value > USB_COMP_EP0_BUFSIZ - 2)
+				value = USB_COMP_EP0_BUFSIZ - 2;
 			buf[0] = (value >> 8) & 0xFF;
 			buf[1] = value & 0xFF;
 			memcpy(buf + 2, *dev->pnp_string, value);
@@ -1269,6 +1280,18 @@ static ssize_t f_printer_opts_pnp_string_store(struct config_item *item,
 
 	mutex_lock(&opts->lock);
 
+	/*
+	 * The string is echoed on the wire by the GET_DEVICE_ID request as
+	 * a two-byte length prefix followed by the string itself, and the
+	 * EP0 response buffer is only USB_COMP_EP0_BUFSIZ bytes, so a
+	 * longer string would make printer_func_setup() overrun that
+	 * buffer.  Reject it here as an additional line of defense.
+	 */
+	if (len > USB_COMP_EP0_BUFSIZ - 2) {
+		result = -EINVAL;
+		goto unlock;
+	}
+
 	new_pnp = kstrndup(page, len, GFP_KERNEL);
 	if (!new_pnp) {
 		result = -ENOMEM;
-- 
2.25.1