[PATCH v1 1/3] usb: gadget: configfs: fix WebUSB landing page missing NUL terminator

[Aristo Chen]

landing_page is sized WEBUSB_URL_RAW_MAX_LENGTH, which is exactly the
length of the longest URL that can be represented in a WebUSB URL
descriptor (U8_MAX - WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + 8 == 260),
leaving no room for a NUL terminator.

webusb_landingPage_store() bounds the URL with

	if (l > sizeof(gi->landing_page))

so l == 260 is accepted, and for a "https://"; URL the second bound
allows 260 as well.  memcpy_and_pad() degenerates to a plain memcpy()
when dest_len == count, so nothing terminates the string:

	printf '%s' "https://$(printf 'A%.0s' $(seq 252))" > webusb/landingPage

configfs runs store() once per write(), so the 260 bytes have to reach
it in a single write to hit the case.

webusb_landingPage_show() then does sysfs_emit(page, "%s\n", ...), which
reads past the end of the array.  What follows landing_page is the
padding in front of the spinlock member, three bytes of it in the
layout here, and kzalloc() left that padding zero, so the read stops
there and the attribute happens to return exactly the bytes that were
written.  The over-read is harmless today only by accident of the
layout: the terminator is never written, and nothing keeps a new member
or a different configuration from putting live data where the zeroed
padding currently sits.

A 260 byte URL is legitimate: after stripping "https://"; it yields a
252 byte URL descriptor payload, or bLength == U8_MAX exactly.  So
rather than rejecting it, give the buffers room for the terminator and
keep WEBUSB_URL_RAW_MAX_LENGTH as what its name says, a maximum URL
length.  The explicit bound in webusb_landingPage_store() now uses that
macro instead of sizeof(), since the buffer is deliberately one byte
larger than the longest URL it may hold.

Fixes: 93c473948c58 ("usb: gadget: add WebUSB landing page support")
Signed-off-by: Aristo Chen <aristo.chen@xxxxxxxxxxxxx>
---
 drivers/usb/gadget/configfs.c | 6 +++---
 include/linux/usb/composite.h | 2 +-
 include/linux/usb/webusb.h    | 5 ++++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 51df6d1d1487..4bc95f4b6670 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -55,7 +55,7 @@ struct gadget_info {
 	bool use_webusb;
 	u16 bcd_webusb_version;
 	u8 b_webusb_vendor_code;
-	char landing_page[WEBUSB_URL_RAW_MAX_LENGTH];
+	char landing_page[WEBUSB_URL_RAW_MAX_LENGTH + 1];
 
 	spinlock_t spinlock;
 	bool unbind;
@@ -1072,7 +1072,7 @@ static ssize_t webusb_landingPage_store(struct config_item *item, const char *pa
 		++bytes_to_strip;
 	}
 
-	if (l > sizeof(gi->landing_page)) {
+	if (l > WEBUSB_URL_RAW_MAX_LENGTH) {
 		pr_err("webusb: landingPage URL too long\n");
 		return -EINVAL;
 	}
@@ -1742,7 +1742,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 		cdev->use_webusb = true;
 		cdev->bcd_webusb_version = gi->bcd_webusb_version;
 		cdev->b_webusb_vendor_code = gi->b_webusb_vendor_code;
-		memcpy(cdev->landing_page, gi->landing_page, WEBUSB_URL_RAW_MAX_LENGTH);
+		memcpy(cdev->landing_page, gi->landing_page, sizeof(cdev->landing_page));
 	}
 
 	if (gi->use_os_desc) {
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index c18041fafa52..0621a6a5cc57 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -472,7 +472,7 @@ struct usb_composite_dev {
 	/* WebUSB */
 	u16				bcd_webusb_version;
 	u8				b_webusb_vendor_code;
-	char				landing_page[WEBUSB_URL_RAW_MAX_LENGTH];
+	char				landing_page[WEBUSB_URL_RAW_MAX_LENGTH + 1];
 	unsigned int			use_webusb:1;
 
 	/* private: */
diff --git a/include/linux/usb/webusb.h b/include/linux/usb/webusb.h
index fe43020b4a48..a3febe726911 100644
--- a/include/linux/usb/webusb.h
+++ b/include/linux/usb/webusb.h
@@ -68,12 +68,15 @@ struct webusb_url_descriptor {
 } __packed;
 
 /*
- * Buffer size to hold the longest URL that can be in an URL descriptor
+ * Length of the longest URL that can be in an URL descriptor
  *
  * The descriptor can be U8_MAX  bytes long.
  * WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH bytes are used for a header.
  * Since the longest prefix that might be stripped is "https://";, we may accommodate an additional
  * 8 bytes.
+ *
+ * Note that this is a string length and not a buffer size: a buffer holding such
+ * a URL needs one more byte for the NUL terminator.
  */
 #define WEBUSB_URL_RAW_MAX_LENGTH (U8_MAX - WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + 8)
 
-- 
2.53.0