[PATCH v1 0/3] usb: gadget: fix WebUSB landing page handling

[PATCH v1 0/3] usb: gadget: fix WebUSB landing page handling

[Aristo Chen]

Three fixes for the WebUSB landing page path, found while reading
webusb_landingPage_store() and the descriptor emission that consumes
what it stores.  The first two are bugs, the third is the cleanup that
made the second one easier to see.

Patch 1 stores the landing page without a NUL terminator when the URL
is exactly WEBUSB_URL_RAW_MAX_LENGTH bytes, because the buffer is sized
to hold the longest legal URL and nothing more.  webusb_landingPage_show()
then reads past the end of that array.  It currently stops in the zeroed
padding before the next member, so nothing is disclosed, but the
terminator that ought to stop it is simply absent.  A 260 byte URL is
legitimate, since after the "https://" prefix is stripped it is exactly
a 255 byte descriptor, so the buffer gains a byte for the terminator
rather than the limit losing one.

Patch 2 fixes the emission side.  The strnlen() bound subtracts the
descriptor header from a field that already excludes it, and the host's
w_length is folded into the URL length, which conflates how much URL
there is, how large the descriptor is, and how many bytes were asked
for.  The result is a URL copy bounded by the request rather than by
URL[], a u8 bLength that wraps for w_length 256..259, a bLength that
describes the transfer instead of the descriptor, and a reply longer
than the data stage for w_length below 3.  Computing the URL length
once and clamping only the reply removes all four.

Patch 3 drops a dead store in webusb_landingPage_store() and renames the
variable to what it actually holds.  No functional change.

The two fixes are independent; patch 2 is correct with or without patch
1 applied.  Only patch 3 depends on ordering, and it is last.

I do not have a WebUSB host handy, so the descriptor arithmetic in patch
2 was checked by transcribing the before and after logic into userspace
and sweeping every (URL, w_length) pair up to USB_COMP_EP0_BUFSIZ: no
copy past URL[], no bLength wrap, no reply exceeding w_length, and
bLength always equal to the true descriptor size.  The store side was
modelled the same way, with the existing behaviour first read off the
webusb/landingPage attribute of a running kernel so the model could be
checked against it: patch 3 changes nothing observable, and patch 1
changes nothing but the terminator.

Aristo Chen (3):
  usb: gadget: configfs: fix WebUSB landing page missing NUL terminator
  usb: gadget: composite: fix WebUSB URL descriptor length handling
  usb: gadget: configfs: drop dead store in webusb_landingPage_store()

 drivers/usb/gadget/composite.c | 33 ++++++++++++++++++---------------
 drivers/usb/gadget/configfs.c  | 22 ++++++++++------------
 include/linux/usb/composite.h  |  2 +-
 include/linux/usb/webusb.h     |  5 ++++-
 4 files changed, 33 insertions(+), 29 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.53.0

[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@canonical.com>
---
 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

[PATCH v1 2/3] usb: gadget: composite: fix WebUSB URL descriptor length handling

[Aristo Chen]

The bound passed to strnlen() subtracts the descriptor header twice:

	landing_page_length = strnlen(cdev->landing_page,
		sizeof(url_descriptor->URL)
		- WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + landing_page_offset);

URL[] is already declared as U8_MAX - WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH
bytes, so it does not include the header, and subtracting the header
again leaves room for three bytes fewer than the descriptor can carry.

That is normally masked by the w_length handling below it, which folds
the host's requested length into the URL length:

	landing_page_length = w_length
		- WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + landing_page_offset;

Doing so conflates three separate quantities, namely how much URL there
is, how large the descriptor is, and how many bytes the host asked for.
It gets all three wrong:

 - the emitted URL length becomes w_length - header, bounded by the
   request rather than by sizeof(url_descriptor->URL), so a host asking
   for w_length between 256 and 259 has up to 256 bytes copied into the
   252 byte URL[] and bLength, a u8, wraps to 0 or 3.  cdev->req->buf is
   USB_COMP_EP0_BUFSIZ bytes, so nothing outside the request buffer is
   touched, but the descriptor is malformed.

 - bLength ends up describing the transfer instead of the descriptor.
   WebUSB defines it as the size of the descriptor, so a full length
   landing page requested with w_length 4 must still report bLength 255
   while transferring four bytes; instead it reports 4.

 - for w_length below WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH the reply is
   the three byte header, which is more than the host's data stage.

Compute the URL length once, bounded only by sizeof(url_descriptor->URL),
build the descriptor from it, and shorten the reply alone with
min_t(u16, w_length, ...) the way the rest of composite_setup() already
does.  A 260 byte "https://" landing page is now emitted as 252 URL
bytes with bLength 255, and short requests are answered with a correctly
sized descriptor truncated to what was asked for.

Fixes: 93c473948c58 ("usb: gadget: add WebUSB landing page support")
Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
 drivers/usb/gadget/composite.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index df39e3487c1f..6c8e15faee6a 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2151,7 +2151,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 		    w_index == WEBUSB_GET_URL &&
 		    w_value == WEBUSB_LANDING_PAGE_PRESENT &&
 		    ctrl->bRequest == cdev->b_webusb_vendor_code) {
-			unsigned int	landing_page_length;
+			unsigned int	url_length;
 			unsigned int	landing_page_offset;
 			struct webusb_url_descriptor *url_descriptor =
 					(struct webusb_url_descriptor *)cdev->req->buf;
@@ -2169,24 +2169,27 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 				url_descriptor->bScheme = WEBUSB_URL_SCHEME_NONE;
 			}
 
-			landing_page_length = strnlen(cdev->landing_page,
-				sizeof(url_descriptor->URL)
-				- WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + landing_page_offset);
-
-			if (w_length < WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH)
-				landing_page_length = landing_page_offset;
-			else if (w_length <
-				 WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + landing_page_length)
-				landing_page_length = w_length
-				- WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + landing_page_offset;
+			/*
+			 * The scheme prefix is encoded in bScheme and is not
+			 * emitted, so URL[] bounds what is left of the URL.
+			 */
+			url_length = strnlen(cdev->landing_page,
+					     sizeof(cdev->landing_page));
+			url_length -= landing_page_offset;
+			if (url_length > sizeof(url_descriptor->URL))
+				url_length = sizeof(url_descriptor->URL);
 
 			memcpy(url_descriptor->URL,
 				cdev->landing_page + landing_page_offset,
-				landing_page_length - landing_page_offset);
-			url_descriptor->bLength = landing_page_length
-				- landing_page_offset + WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH;
+				url_length);
+			url_descriptor->bLength = url_length
+				+ WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH;
 
-			value = url_descriptor->bLength;
+			/*
+			 * bLength describes the descriptor, not the transfer,
+			 * so only the reply is shortened to what was asked for.
+			 */
+			value = min_t(u16, w_length, url_descriptor->bLength);
 
 			goto check_value;
 		}
-- 
2.53.0

[PATCH v1 3/3] usb: gadget: configfs: drop dead store in webusb_landingPage_store()

[Aristo Chen]

bytes_to_strip is used for two unrelated purposes in
webusb_landingPage_store(). It is first incremented to account for a
trailing newline:

	if (page[l - 1] == '\n') {
		--l;
		++bytes_to_strip;
	}

and then unconditionally overwritten a few lines later by the URL scheme
detection, so the increment is a dead store: the newline has already
been accounted for by --l.

The dead store makes the subsequent bound check

	if (l > U8_MAX - WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + bytes_to_strip)

read as though it also allows for the newline when it does not, so
anyone auditing that bound has to first work out that one of the two
meanings of bytes_to_strip is dead. Compilers do not warn about this
because the variable is genuinely used later.

Drop the increment and rename the variable to scheme_len, which is what
it actually holds. No functional change.

Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
 drivers/usb/gadget/configfs.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 4bc95f4b6670..02e619ed14f5 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1062,15 +1062,13 @@ static ssize_t webusb_landingPage_store(struct config_item *item, const char *pa
 				     size_t len)
 {
 	struct gadget_info *gi = webusb_item_to_gadget_info(item);
-	unsigned int bytes_to_strip = 0;
+	unsigned int scheme_len;
 	int l = len;
 
 	if (!len)
 		return len;
-	if (page[l - 1] == '\n') {
+	if (page[l - 1] == '\n')
 		--l;
-		++bytes_to_strip;
-	}
 
 	if (l > WEBUSB_URL_RAW_MAX_LENGTH) {
 		pr_err("webusb: landingPage URL too long\n");
@@ -1079,15 +1077,15 @@ static ssize_t webusb_landingPage_store(struct config_item *item, const char *pa
 
 	// validation
 	if (strncasecmp(page, "https://",  8) == 0)
-		bytes_to_strip = 8;
+		scheme_len = 8;
 	else if (strncasecmp(page, "http://", 7) == 0)
-		bytes_to_strip = 7;
+		scheme_len = 7;
 	else
-		bytes_to_strip = 0;
+		scheme_len = 0;
 
-	if (l > U8_MAX - WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + bytes_to_strip) {
+	if (l > U8_MAX - WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH + scheme_len) {
 		pr_err("webusb: landingPage URL %d bytes too long for given URL scheme\n",
-			l - U8_MAX + WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH - bytes_to_strip);
+			l - U8_MAX + WEBUSB_URL_DESCRIPTOR_HEADER_LENGTH - scheme_len);
 		return -EINVAL;
 	}
 
-- 
2.53.0

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

[Alan Stern]

On Mon, Aug 31, 2026 at 03:39:37PM +0000, Aristo Chen wrote:
> 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@canonical.com>
> ---

A somewhat verbose description, but okay.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  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
> 
>