* [PATCH v3] usb: gadget: f_printer: fix OOB write and UAF of pnp_string in printer_func_setup()
2026-09-19 8:08 ` [PATCH v2] [USB][gadget/f_printer] Fix 3074-byte heap buffer overflow and UAF in printer_func_setup() Hui Peng
@ 2026-09-19 11:28 ` Hui Peng
0 siblings, 0 replies; 2+ messages in thread
From: Hui Peng @ 2026-09-19 11:28 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, linux-kernel
printer_func_setup(GET_DEVICE_ID) copies strlen(dev->pnp_string) (up to
PAGE_SIZE = 4096 bytes via configfs pnp_string_store) into the 1024-byte
cdev->req->buf (USB_COMP_EP0_BUFSIZ) without clamping to
USB_COMP_EP0_BUFSIZ, and caches a raw pointer to opts->pnp_string that is
freed when configfs updates pnp_string. Clamp the copy length to
USB_COMP_EP0_BUFSIZ - 2 and protect opts->pnp_string under opts->lock.
Kernel stack trace:
BUG: KASAN: slab-out-of-bounds in printer_func_setup+0x28c/0x490
Write of size 3072 at addr ffff888007184402 by task irq/18-dummy_hc/64
Call Trace:
<IRQ>
dump_stack_lvl+0x4d/0x70
print_report+0xc4/0x610
kasan_report+0xb8/0xf0
kasan_check_range+0x118/0x190
memcpy+0x3c/0x60
printer_func_setup+0x28c/0x490
composite_setup+0x412/0x2890
dummy_timer+0x894/0x1420
__hrtimer_run_queues+0x218/0x540
hrtimer_interrupt+0x1a4/0x380
</IRQ>
Kernel stack trace (Linux 7.3.0-rc3):
==================================================================
BUG: KASAN: slab-out-of-bounds in printer_func_setup+0xa20/0xf40
Write of size 4095 at addr ffff88800f46c002 by task kworker/0:2/69
Call Trace:
<TASK>
dump_stack_lvl+0x70/0xa0
print_report+0x153/0x4c6
kasan_report+0xf1/0x120
kasan_check_range+0x11c/0x200
__asan_memcpy+0x40/0x70
printer_func_setup+0xa20/0xf40
composite_setup+0x67b/0x4f70
</TASK>
==================================================================
Fixes: fdc01cc286be ("usb: gadget: printer: Remove pnp_string static buffer")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
v3: Add a Fixes: tag, fix the subject prefix, correct the overflow size
in the subject, and - importantly - flag a problem with my own patch
that I would like guidance on before you take it.
Fixes: tag. fdc01cc286be ("usb: gadget: printer: Remove pnp_string
static buffer") is where both halves of this start. Before it, value
came from the two on-wire length bytes and the string lived in a static
1024-byte buffer, so neither the unbounded copy nor the
free-while-in-use was possible. That commit replaced the length bytes
with strlen(), dropped the w_length <= PNP_STRING_LEN test, and made
the string a kstrndup()'d allocation owned by configfs. 24b7ba2f88e0
("usb: gadget: function: fix dangling pnp_string in f_printer.c") only
converted char * to char **, and itself carries Fixes: fdc01cc286be.
Subject. v1/v2 said "3074-byte heap buffer overflow", which is wrong
for current mainline and I should not have left it there.
ed769520727e ("usb: gadget: composite Allow for larger configuration
descriptors") raised USB_COMP_EP0_BUFSIZ from 1024 to 4096 in v4.17, so
on 7.3.0-rc3 the overflow is only 1-2 bytes past req->buf. 3074 bytes
is the worst case on v4.11..v4.16, which is where my first trace came
from.
Now the problem with the patch. The spin_lock(&dev->lock) I added
around the pnp_string access does NOT actually serialise against the
free: opts->pnp_string is kfree()d in
f_printer_opts_pnp_string_store() under the opts->lock mutex, and
dev->lock is a different (spin) lock entirely. So the locking hunk is
decorative with respect to the use-after-free. What really closes the
UAF in this patch is the opts->refcnt -> -EBUSY check in the store
path - but that contradicts the intent stated in 24b7ba2f88e0, namely
that "the ieee 1284 id string should be allowed to change while the
device is connected".
So there is a real design choice here that is yours to make, not mine:
a) keep -EBUSY and accept that the ID string becomes immutable once
the function is bound (simple, but a behaviour regression), or
b) drop -EBUSY, take dev->lock in the store path as well, and
free the old string outside the spinlock, or
c) snapshot the string into the f_printer_dev at gprinter_alloc()
time so setup() never touches configfs-owned memory at all.
I would lean towards (c), but I did not want to send a larger rework
under a Fixes: tag without asking. Tell me which you want and I will
send a v4. The bounds-check half of the patch is independent of this
and correct either way.
Backporting: this hunk is written against the post-v6.1 form where
dev->pnp_string is a char ** (24b7ba2f88e0). Older stable trees still
have a plain char * and need the extra dereference dropped; they are
affected too, and more severely, per the buffer-size note above.
v2: Send to linux-usb@vger.kernel.org via git send-email with Assisted-by: LLM tag.
drivers/usb/gadget/function/f_printer.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 1857d7861..0b505a760 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -1030,16 +1030,19 @@ static int printer_func_setup(struct usb_function *f,
if ((wIndex>>8) != dev->interface)
break;
- if (!*dev->pnp_string) {
+ spin_lock(&dev->lock);
+ if (!dev->pnp_string || !*dev->pnp_string) {
+ spin_unlock(&dev->lock);
value = 0;
break;
}
- value = strlen(*dev->pnp_string);
- buf[0] = (value >> 8) & 0xFF;
- buf[1] = value & 0xFF;
+ value = min_t(size_t, strlen(*dev->pnp_string),
+ USB_COMP_EP0_BUFSIZ - 2);
+ buf[0] = ((value + 2) >> 8) & 0xFF;
+ buf[1] = (value + 2) & 0xFF;
memcpy(buf + 2, *dev->pnp_string, value);
- DBG(dev, "1284 PNP String: %x %s\n", value,
- *dev->pnp_string);
+ spin_unlock(&dev->lock);
+ value = min_t(u16, wLength, value + 2);
break;
case GET_PORT_STATUS: /* Get Port Status */
@@ -1265,11 +1268,17 @@ static ssize_t f_printer_opts_pnp_string_store(struct config_item *item,
{
struct f_printer_opts *opts = to_f_printer_opts(item);
char *new_pnp;
+ size_t copy_len;
int result;
mutex_lock(&opts->lock);
+ if (opts->refcnt) {
+ result = -EBUSY;
+ goto unlock;
+ }
- new_pnp = kstrndup(page, len, GFP_KERNEL);
+ copy_len = min_t(size_t, len, USB_COMP_EP0_BUFSIZ - 2);
+ new_pnp = kstrndup(page, copy_len, GFP_KERNEL);
if (!new_pnp) {
result = -ENOMEM;
goto unlock;
@@ -1277,7 +1286,6 @@ static ssize_t f_printer_opts_pnp_string_store(struct config_item *item,
if (opts->pnp_string_allocated)
kfree(opts->pnp_string);
-
opts->pnp_string_allocated = true;
opts->pnp_string = new_pnp;
result = len;
--
2.55.0.1082.g2b9226bbc0-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread