Skip to contentSkip to conversation

usb: gadget: configfs: fix OOB read in ext_prop_data_show()

PATCH In review Bingquan Chen 2 participants 2 messages Apr 22 – May 22

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 usb: gadget: configfs: fix OOB read in ext_prop_data_show()

In ext_prop_data_store(), for unicode property types, the data buffer is allocated via kmemdup() with size 'len', but data_len is set to len*2+2 to account for the UTF-16 encoding and a 2-byte null terminator, as required by the Microsoft OS Extended Properties Descriptor specification (dwPropertyDataLength must include the terminator).

However, the null terminator is never actually stored in the data buffer. When ext_prop_data_show() reads the data back, it computes the read length as data_len >> 1 = len+1, then does memcpy(page, data, len+1), reading 1 byte past the allocated buffer. This is a slab-out-of-bounds read that leaks 1 byte of adjacent heap data to userspace via configfs.

KASAN report (5.10.252):

  BUG: KASAN: slab-out-of-bounds in ext_prop_data_show+0x4a/0x60
  Read of size 9 at addr ffff888005546008 by task poc/62

  Allocated by task 62:
   kmemdup+0x17/0x40
   ext_prop_data_store+0x52/0x130
   configfs_write_file+0x168/0x200

  The buggy address belongs to the object at ffff888005546008
   which belongs to the cache kmalloc-8 of size 8

Fix by allocating len+2 bytes and explicitly zero-terminating with a full 2-byte UTF-16 null terminator. This ensures the buffer fully matches the dwPropertyDataLength semantics (len*2+2) while eliminating the OOB read.

Fixes 7419485f197c ("usb: gadget: configfs: OS Extended Properties descriptors support")
---
 drivers/usb/gadget/configfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/configfs.c +4 -1
index 183a25f65ac8..b2c3d4e5f6a7 100644
@@ -1352,8 +1352,12 @@ 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 = kmalloc(len + 2, GFP_KERNEL);
 	if (!new_data)
 		return -ENOMEM;
+	memcpy(new_data, page, len);
+	new_data[len]     = '\0';
+	new_data[len + 1] = '\0';
 
 	if (desc->opts_mutex)

--
2.43.0

#1 lore raw
Quoted 57 lines · Bingquan Chen

In ext_prop_data_store(), for unicode property types, the data buffer is allocated via kmemdup() with size 'len', but data_len is set to len*2+2 to account for the UTF-16 encoding and a 2-byte null terminator, as required by the Microsoft OS Extended Properties Descriptor specification (dwPropertyDataLength must include the terminator).

However, the null terminator is never actually stored in the data buffer. When ext_prop_data_show() reads the data back, it computes the read length as data_len >> 1 = len+1, then does memcpy(page, data, len+1), reading 1 byte past the allocated buffer. This is a slab-out-of-bounds read that leaks 1 byte of adjacent heap data to userspace via configfs.

KASAN report (5.10.252):

  BUG: KASAN: slab-out-of-bounds in ext_prop_data_show+0x4a/0x60
  Read of size 9 at addr ffff888005546008 by task poc/62

  Allocated by task 62:
   kmemdup+0x17/0x40
   ext_prop_data_store+0x52/0x130
   configfs_write_file+0x168/0x200

  The buggy address belongs to the object at ffff888005546008
   which belongs to the cache kmalloc-8 of size 8

Fix by allocating len+2 bytes and explicitly zero-terminating with a full 2-byte UTF-16 null terminator. This ensures the buffer fully matches the dwPropertyDataLength semantics (len*2+2) while eliminating the OOB read.

Fixes 7419485f197c ("usb: gadget: configfs: OS Extended Properties descriptors support")
---
 drivers/usb/gadget/configfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/configfs.c +4 -2
index 183a25f65ac8..b2c3d4e5f6a7 100644
@@ -1352,8 +1352,12 @@ 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 = kmalloc(len + 2, GFP_KERNEL);
 	if (!new_data)
 		return -ENOMEM;
+	memcpy(new_data, page, len);
+	new_data[len]     = '\0';
+	new_data[len + 1] = '\0';
 
 	if (desc->opts_mutex)
--
Quoted 1 line

2.43.0

This patch is corrupted and can not be applied :(