[PATCH] usb: gadget: configfs: NUL-terminate extended property data

[PATCH] usb: gadget: configfs: NUL-terminate extended property data

[Yingjie Wang]

Unicode extended properties account for a UTF-16 terminator in
ext_prop->data_len.  The configfs store path currently allocates only the
input bytes, so ext_prop_data_show() copies one byte past the allocation.

Use kmemdup_nul() for the configfs data so the source byte implied by the
terminator-inclusive length is present for both the configfs readback and
the descriptor conversion.

Fixes: 7419485f197c ("usb: gadget: configfs: OS Extended Properties descriptors support")

Reported-by: Bingquan Chen <patzilla007@gmail.com>
Link: https://lore.kernel.org/all/20260421141010.5607-1-patzilla007@gmail.com/
Cc: stable@vger.kernel.org

Tested: x86-64 KASAN configfs readback through a built-in RNDIS function
instance.  The baseline reports a slab-out-of-bounds read in
ext_prop_data_show(); the patched kernel reads back the expected 0x41 0x00
bytes with no KASAN report.

Signed-off-by: Yingjie Wang <1075151112@qq.com>
---
 drivers/usb/gadget/configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 51df6d1d1487a..c302e8dd25a78 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1351,7 +1351,7 @@ static ssize_t ext_prop_data_store(struct config_item *item,
 
 	if (page[len - 1] == '\n' || page[len - 1] == '\0')
 		--len;
-	new_data = kmemdup(page, len, GFP_KERNEL);
+	new_data = kmemdup_nul(page, len, GFP_KERNEL);
 	if (!new_data)
 		return -ENOMEM;
 

base-commit: 6b4b543fa18757297ac756a6fe6e64f506b7844a
-- 
2.43.0

Re: [PATCH] usb: gadget: configfs: NUL-terminate extended property data

[Neill Kapron]

> Unicode extended properties account for a UTF-16 terminator in
> ext_prop->data_len.  The configfs store path currently allocates only the
> input bytes, so ext_prop_data_show() copies one byte past the allocation.
> 
> Use kmemdup_nul() for the configfs data so the source byte implied by the
> terminator-inclusive length is present for both the configfs readback and
> the descriptor conversion.
> 
> Fixes: 7419485f197c ("usb: gadget: configfs: OS Extended Properties descriptors support")
> 
> Reported-by: Bingquan Chen <patzilla007@gmail.com>
> Link: https://lore.kernel.org/all/20260421141010.5607-1-patzilla007@gmail.com/
> Cc: stable@vger.kernel.org
> 
> Tested: x86-64 KASAN configfs readback through a built-in RNDIS function
> instance.  The baseline reports a slab-out-of-bounds read in
> ext_prop_data_show(); the patched kernel reads back the expected 0x41 0x00
> bytes with no KASAN report.
> 
> Signed-off-by: Yingjie Wang <1075151112@qq.com>
> ---
>  drivers/usb/gadget/configfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 51df6d1d1487..c302e8dd25a7 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1351,7 +1351,7 @@ static ssize_t ext_prop_data_store(struct config_item *item,
>  
>  	if (page[len - 1] == '\n' || page[len - 1] == '\0')
>  		--len;
> -	new_data = kmemdup(page, len, GFP_KERNEL);
> +	new_data = kmemdup_nul(page, len, GFP_KERNEL);

While this fixes one case, won't it cause ext_prop_data() show to report
a larger size with the trailing NUL?

The OOM still exists when switching types in ext_prop_type_store().

Can we add a couple of small helpers to convert between the raw payload
length and the wire length and use them in ext_prop_data_store/show,
and ext_prop_type_store to standardize and sanitize the length?

-- 
Neill Kapron <nkapron@google.com>

[PATCH v2 0/2] usb: gadget: configfs: fix extended property length handling

[Yingjie Wang]

The configfs implementation of Microsoft OS Extended Properties keeps the
raw property payload in memory while data_len records the descriptor wire
length.  The Unicode wire length includes a two-byte terminator, but the
stored allocation did not.  Configfs readback and descriptor conversion
could consequently read one byte past the allocation.

Type changes also updated data_len with shifts that omitted the Unicode
terminator and left the aggregate descriptor length unchanged.  This could
truncate a property value, report a stale dwLength, or make configfs
readback use a length larger than the allocation.

Patch 1 allocates the source terminator, guards empty writes and introduces
the raw-to-wire length helper.  Patch 2 keeps the raw data length separately,
derives Unicode wire lengths from the actual UTF-16 conversion, uses both
lengths for type changes and readback, and updates the aggregate descriptor
length.  Configfs now returns the raw payload, while the host-visible Unicode
descriptor retains its UTF-16 terminator.

Changes since v1:
- use separate raw and wire lengths instead of inferring UTF-8 bytes from
  a UTF-16 wire length;
- derive Unicode wire lengths from UTF-16 code units, including the
  terminator, so non-ASCII values are not padded with extra zero units;
- return only the raw payload through configfs readback;
- keep data_len and the aggregate descriptor length consistent across type
  changes;
- calculate the type-dependent length and publish the new data under
  opts_mutex;
- guard empty data writes before checking the trailing byte;
- preserve the FunctionFS Unicode conversion input length while adapting the
  shared serializer;
- reject malformed UTF-8 without replacing the existing property data or
  changing its descriptor lengths;
- include the related type-change fix as patch 2/2.

Tested on x86-64 with KASAN using an RNDIS configfs gadget on dummy_hcd.
The base kernel reported out-of-bounds reads in configfs readback and host
descriptor generation.  Patch 1 removed the direct Unicode-data reports;
the full series also removed the type-change report, with no KASAN, BUG or
WARNING.  Binary-to-Unicode, Unicode-to-binary, data-before-type, property
removal and type 7 configfs readback were exercised.  ASCII, U+00E9 and
U+1F600 values produced exact UTF-16 data lengths and terminators, including
through a binary-to-Unicode transition.  Every host descriptor length
matched the returned response, and configfs returned the raw bytes.  An
invalid UTF-8 write failed without changing the old data, type or host
descriptor.  A separate FunctionFS run returned the complete "abc\0" Unicode
property and preserved a four-byte Binary property containing an embedded
NUL.  Both commits also built with W=1 for x86-64 and i386.

Yingjie Wang (2):
  usb: gadget: configfs: NUL-terminate extended property data
  usb: gadget: configfs: keep extended property lengths consistent

 drivers/usb/gadget/composite.c     |   1 +
 drivers/usb/gadget/configfs.c      | 100 +++++++++++++++++++----------
 drivers/usb/gadget/function/f_fs.c |   1 +
 drivers/usb/gadget/u_os_desc.h     |   8 +--
 include/linux/usb/composite.h      |   4 +-
 5 files changed, 76 insertions(+), 38 deletions(-)


base-commit: e971f4f365bad51d01bc90db55b56fdb3124f963
-- 
2.43.0

[PATCH v2 1/2] usb: gadget: configfs: NUL-terminate extended property data

[Yingjie Wang]

Unicode extended properties include a UTF-16 terminator in their wire
length.  The configfs store path keeps the raw UTF-8 bytes, but the
configfs readback and descriptor conversion consume the source byte
implied by that terminator-inclusive length.

The allocation currently contains only the input bytes, so both paths
can read one byte past it.  Allocate the trailing NUL with kmemdup_nul()
and centralize the raw-to-wire length conversion used by the store path.
Avoid dereferencing page[-1] on an empty configfs write.

Fixes: 7419485f197c ("usb: gadget: configfs: OS Extended Properties descriptors support")
Reported-by: Bingquan Chen <patzilla007@gmail.com>
Link: https://lore.kernel.org/r/20260421141010.5607-1-patzilla007@gmail.com
Suggested-by: Neill Kapron <nkapron@google.com>
Link: https://lore.kernel.org/r/179338899796.2590875.15337207674245295434.b4-review@b4
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 drivers/usb/gadget/configfs.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 51df6d1d1487a..f5fff213d7952 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1283,6 +1283,21 @@ static inline struct usb_os_desc_ext_prop
 	return container_of(item, struct usb_os_desc_ext_prop, item);
 }
 
+static bool ext_prop_is_unicode_string(u8 type)
+{
+	return type == USB_EXT_PROP_UNICODE ||
+	       type == USB_EXT_PROP_UNICODE_ENV ||
+	       type == USB_EXT_PROP_UNICODE_LINK;
+}
+
+static int ext_prop_data_wire_len(u8 type, int raw_len)
+{
+	if (ext_prop_is_unicode_string(type))
+		return 2 * raw_len + 2;
+
+	return raw_len;
+}
+
 static ssize_t ext_prop_type_show(struct config_item *item, char *page)
 {
 	return sprintf(page, "%d\n", to_usb_os_desc_ext_prop(item)->type);
@@ -1332,9 +1347,7 @@ static ssize_t ext_prop_data_show(struct config_item *item, char *page)
 	struct usb_os_desc_ext_prop *ext_prop = to_usb_os_desc_ext_prop(item);
 	int len = ext_prop->data_len;
 
-	if (ext_prop->type == USB_EXT_PROP_UNICODE ||
-	    ext_prop->type == USB_EXT_PROP_UNICODE_ENV ||
-	    ext_prop->type == USB_EXT_PROP_UNICODE_LINK)
+	if (ext_prop_is_unicode_string(ext_prop->type))
 		len >>= 1;
 	memcpy(page, ext_prop->data, len);
 
@@ -1349,9 +1362,9 @@ static ssize_t ext_prop_data_store(struct config_item *item,
 	char *new_data;
 	size_t ret_len = len;
 
-	if (page[len - 1] == '\n' || page[len - 1] == '\0')
+	if (len && (page[len - 1] == '\n' || page[len - 1] == '\0'))
 		--len;
-	new_data = kmemdup(page, len, GFP_KERNEL);
+	new_data = kmemdup_nul(page, len, GFP_KERNEL);
 	if (!new_data)
 		return -ENOMEM;
 
@@ -1360,16 +1373,8 @@ static ssize_t ext_prop_data_store(struct config_item *item,
 	kfree(ext_prop->data);
 	ext_prop->data = new_data;
 	desc->ext_prop_len -= ext_prop->data_len;
-	ext_prop->data_len = len;
+	ext_prop->data_len = ext_prop_data_wire_len(ext_prop->type, len);
 	desc->ext_prop_len += ext_prop->data_len;
-	if (ext_prop->type == USB_EXT_PROP_UNICODE ||
-	    ext_prop->type == USB_EXT_PROP_UNICODE_ENV ||
-	    ext_prop->type == USB_EXT_PROP_UNICODE_LINK) {
-		desc->ext_prop_len -= ext_prop->data_len;
-		ext_prop->data_len <<= 1;
-		ext_prop->data_len += 2;
-		desc->ext_prop_len += ext_prop->data_len;
-	}
 	if (desc->opts_mutex)
 		mutex_unlock(desc->opts_mutex);
 	return ret_len;
-- 
2.43.0

[PATCH v2 2/2] usb: gadget: configfs: keep extended property lengths consistent

[Yingjie Wang]

ext_prop->data_len is the descriptor wire length, while the configfs data
attribute contains the raw payload.  ext_prop_type_store() converts between
property types with shifts that omit the Unicode terminator and does not
update desc->ext_prop_len.  The resulting descriptor can have a truncated
value and a stale total length.  Transitions involving type 7 can also
leave data_len larger than the allocation used by configfs readback.

Keep the raw data length separately from the wire length.  Type changes and
configfs readback then avoid treating UTF-16 wire length as a count of
UTF-8 bytes.  For Unicode data, calculate the wire length from the UTF-16
code units produced by the UTF-8 input, including the terminator, and pass
the raw length to the serializer.  This avoids padding non-ASCII values
with extra zero code units and keeps the descriptor length fields
consistent.  For FunctionFS properties, record the original descriptor
data length as the source length, preserving the existing conversion bound.
Calculate the new wire length while holding opts_mutex so a concurrent type
change cannot pair the new data with a length for the old type.

Reject malformed UTF-8 with -EINVAL before replacing the stored payload,
rather than accepting data that the serializer cannot encode.  A failed
write therefore leaves the existing data and length accounting unchanged.

Type changes now calculate the wire length for the new type and update the
aggregate descriptor length.  Configfs readback returns the raw payload
rather than the descriptor's UTF-16 terminator.

Fixes: 7419485f197c ("usb: gadget: configfs: OS Extended Properties descriptors support")
Suggested-by: Neill Kapron <nkapron@google.com>
Link: https://lore.kernel.org/r/179338899796.2590875.15337207674245295434.b4-review@b4
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 drivers/usb/gadget/composite.c     |  1 +
 drivers/usb/gadget/configfs.c      | 79 ++++++++++++++++++++----------
 drivers/usb/gadget/function/f_fs.c |  1 +
 drivers/usb/gadget/u_os_desc.h     |  8 +--
 include/linux/usb/composite.h      |  4 +-
 5 files changed, 63 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 1b2238147cfe3..06053f5ae1947 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1721,6 +1721,7 @@ static int fill_ext_prop(struct usb_configuration *c, int interface, u8 *buf)
 				case USB_EXT_PROP_UNICODE_LINK:
 					usb_ext_prop_put_unicode(buf, ret,
 							 ext_prop->data,
+							 ext_prop->data_raw_len,
 							 ext_prop->data_len);
 					break;
 				case USB_EXT_PROP_BINARY:
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index f5fff213d7952..3d931da3ae6b4 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -4,6 +4,7 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/kstrtox.h>
+#include <linux/limits.h>
 #include <linux/nls.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/func_utils.h>
@@ -1290,12 +1291,34 @@ static bool ext_prop_is_unicode_string(u8 type)
 	       type == USB_EXT_PROP_UNICODE_LINK;
 }
 
-static int ext_prop_data_wire_len(u8 type, int raw_len)
+static int ext_prop_data_wire_len(u8 type, const char *data, int raw_len)
 {
-	if (ext_prop_is_unicode_string(type))
-		return 2 * raw_len + 2;
+	const u8 *s = (const u8 *)data;
+	int wire_len = 2;
 
-	return raw_len;
+	if (!ext_prop_is_unicode_string(type))
+		return raw_len;
+
+	while (raw_len > 0 && *s) {
+		unicode_t u;
+		int size;
+
+		if (!(*s & 0x80)) {
+			s++;
+			raw_len--;
+			wire_len += 2;
+			continue;
+		}
+
+		size = utf8_to_utf32(s, raw_len, &u);
+		if (size < 0)
+			return -EINVAL;
+		s += size;
+		raw_len -= size;
+		wire_len += u > U16_MAX ? 4 : 2;
+	}
+
+	return wire_len;
 }
 
 static ssize_t ext_prop_type_show(struct config_item *item, char *page)
@@ -1321,37 +1344,34 @@ static ssize_t ext_prop_type_store(struct config_item *item,
 	if (desc->opts_mutex)
 		mutex_lock(desc->opts_mutex);
 
-	if ((ext_prop->type == USB_EXT_PROP_BINARY ||
-	    ext_prop->type == USB_EXT_PROP_LE32 ||
-	    ext_prop->type == USB_EXT_PROP_BE32) &&
-	    (type == USB_EXT_PROP_UNICODE ||
-	    type == USB_EXT_PROP_UNICODE_ENV ||
-	    type == USB_EXT_PROP_UNICODE_LINK))
-		ext_prop->data_len <<= 1;
-	else if ((ext_prop->type == USB_EXT_PROP_UNICODE ||
-		   ext_prop->type == USB_EXT_PROP_UNICODE_ENV ||
-		   ext_prop->type == USB_EXT_PROP_UNICODE_LINK) &&
-		   (type == USB_EXT_PROP_BINARY ||
-		   type == USB_EXT_PROP_LE32 ||
-		   type == USB_EXT_PROP_BE32))
-		ext_prop->data_len >>= 1;
+	if (ext_prop->data) {
+		int data_len;
+
+		data_len = ext_prop_data_wire_len(type, ext_prop->data,
+						  ext_prop->data_raw_len);
+		if (data_len < 0) {
+			ret = data_len;
+			goto out;
+		}
+		desc->ext_prop_len += data_len - ext_prop->data_len;
+		ext_prop->data_len = data_len;
+	}
 	ext_prop->type = type;
+	ret = len;
 
+out:
 	if (desc->opts_mutex)
 		mutex_unlock(desc->opts_mutex);
-	return len;
+	return ret;
 }
 
 static ssize_t ext_prop_data_show(struct config_item *item, char *page)
 {
 	struct usb_os_desc_ext_prop *ext_prop = to_usb_os_desc_ext_prop(item);
-	int len = ext_prop->data_len;
 
-	if (ext_prop_is_unicode_string(ext_prop->type))
-		len >>= 1;
-	memcpy(page, ext_prop->data, len);
+	memcpy(page, ext_prop->data, ext_prop->data_raw_len);
 
-	return len;
+	return ext_prop->data_raw_len;
 }
 
 static ssize_t ext_prop_data_store(struct config_item *item,
@@ -1361,6 +1381,7 @@ static ssize_t ext_prop_data_store(struct config_item *item,
 	struct usb_os_desc *desc = to_usb_os_desc(ext_prop->item.ci_parent);
 	char *new_data;
 	size_t ret_len = len;
+	int data_len;
 
 	if (len && (page[len - 1] == '\n' || page[len - 1] == '\0'))
 		--len;
@@ -1370,10 +1391,18 @@ static ssize_t ext_prop_data_store(struct config_item *item,
 
 	if (desc->opts_mutex)
 		mutex_lock(desc->opts_mutex);
+	data_len = ext_prop_data_wire_len(ext_prop->type, new_data, len);
+	if (data_len < 0) {
+		if (desc->opts_mutex)
+			mutex_unlock(desc->opts_mutex);
+		kfree(new_data);
+		return data_len;
+	}
 	kfree(ext_prop->data);
 	ext_prop->data = new_data;
 	desc->ext_prop_len -= ext_prop->data_len;
-	ext_prop->data_len = ext_prop_data_wire_len(ext_prop->type, len);
+	ext_prop->data_raw_len = len;
+	ext_prop->data_len = data_len;
 	desc->ext_prop_len += ext_prop->data_len;
 	if (desc->opts_mutex)
 		mutex_unlock(desc->opts_mutex);
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index c64a268e98a43..c8f68a8049725 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -3583,6 +3583,7 @@ static int __ffs_func_bind_do_os_desc(enum ffs_os_desc_type type,
 		ext_prop->name_len = le16_to_cpu(desc->wPropertyNameLength);
 		ext_prop->data_len = le32_to_cpu(*(__le32 *)
 			usb_ext_prop_data_len_ptr(data, ext_prop->name_len));
+		ext_prop->data_raw_len = ext_prop->data_len;
 		length = ext_prop->name_len + ext_prop->data_len + 14;
 
 		ext_prop_name = func->ffs->ms_os_descs_ext_prop_name_avail;
diff --git a/drivers/usb/gadget/u_os_desc.h b/drivers/usb/gadget/u_os_desc.h
index f8b9f0faa9b16..c4384bfa8fe80 100644
--- a/drivers/usb/gadget/u_os_desc.h
+++ b/drivers/usb/gadget/u_os_desc.h
@@ -101,13 +101,13 @@ static inline void usb_ext_prop_put_binary(u8 *buf, int pnl, const u8 *data,
 }
 
 static inline int usb_ext_prop_put_unicode(u8 *buf, int pnl, const char *string,
-					   int data_len)
+					   int raw_len, int data_len)
 {
 	int result;
 	put_unaligned_le32(data_len, usb_ext_prop_data_len_ptr(buf, pnl));
-	result = utf8s_to_utf16s(string, data_len >> 1, UTF16_LITTLE_ENDIAN,
-			(wchar_t *) usb_ext_prop_data_ptr(buf, pnl),
-			data_len - 2);
+	result = utf8s_to_utf16s(string, raw_len, UTF16_LITTLE_ENDIAN,
+				 (wchar_t *)usb_ext_prop_data_ptr(buf, pnl),
+				 (data_len - 2) / 2);
 	if (result < 0)
 		return result;
 
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index c18041fafa521..1a6db7fb6e3c2 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -61,7 +61,8 @@ struct usb_configuration;
  * @type: Extended Property type
  * @name_len: Extended Property unicode name length, including terminating '\0'
  * @name: Extended Property name
- * @data_len: Length of Extended Property blob (for unicode store double len)
+ * @data_len: Length of the Extended Property data on the wire
+ * @data_raw_len: Length of the raw Extended Property data
  * @data: Extended Property blob
  * @item: Represents this Extended Property in configfs
  */
@@ -71,6 +72,7 @@ struct usb_os_desc_ext_prop {
 	int			name_len;
 	char			*name;
 	int			data_len;
+	int			data_raw_len;
 	char			*data;
 	struct config_item	item;
 };
-- 
2.43.0