[PATCH] usb: gadget: f_printer: fix pnp_string use-after-free on reconfigure

[PATCH] usb: gadget: f_printer: fix pnp_string use-after-free on reconfigure

[Yu Zhang]

f_printer_opts_pnp_string_store() replaces opts->pnp_string with
kfree(old) + kstrndup(new) under opts->lock only, with no opts->refcnt
guard, so the IEEE-1284 ID string can be changed while the function is
bound and the gadget is enumerated.

printer_func_setup() answers the host GET_DEVICE_ID class request from
(soft)irq context and reads the string with no lock:

	value = strlen(*dev->pnp_string);
	...
	memcpy(buf + 2, *dev->pnp_string, value);

dev->pnp_string points at &opts->pnp_string, so a concurrent configfs
write that frees the old string races the setup read -> use-after-free.

KASAN, a configfs writer racing a host GET_DEVICE_ID (reproduced over
dummy_hcd):

  BUG: KASAN: slab-use-after-free in strlen+0xe/0x40
  Read of size 1 at addr ffff888005036000 by task swapper/1/0
  Call Trace:
   <IRQ>
   strlen+0xe/0x40
   printer_func_setup+0x410/0x890
   composite_setup+0x67e/0x4690
   configfs_composite_setup+0xbb/0xd0
   dummy_timer+0x1934/0x1e40
   __hrtimer_run_queues+0x1b6/0x410
   hrtimer_run_softirq+0x13a/0x260
  Allocated by task 3486:
   kstrndup+0xb2/0x130
   f_printer_opts_pnp_string_store+0x3f/0xf0
   configfs_write_iter+0x1a7/0x250
   vfs_write+0x5df/0x940
   ksys_write+0xfd/0x1e0

The setup path runs in atomic context and cannot take opts->lock, so
serialize by refusing to change pnp_string while the function is
instantiated, exactly as the sibling f_printer_opts_q_len_store() already
does for q_len.  While the function is bound the string is then stable
for the lifetime of the live reader; it can still be changed when the
function is not linked into a gadget.

Fixes: 24b7ba2f88e0 ("usb: gadget: function: fix dangling pnp_string in f_printer.c")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
Tested-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/usb/gadget/function/f_printer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 1111111..2222222 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -1269,6 +1269,11 @@ static ssize_t f_printer_opts_pnp_string_store(struct config_item *item,

 	mutex_lock(&opts->lock);

+	if (opts->refcnt) {
+		result = -EBUSY;
+		goto unlock;
+	}
+
 	new_pnp = kstrndup(page, len, GFP_KERNEL);
 	if (!new_pnp) {
 		result = -ENOMEM;
--
2.43.0

Re: [PATCH] usb: gadget: f_printer: fix pnp_string use-after-free on reconfigure

[sashiko-bot]

> f_printer_opts_pnp_string_store() replaces opts->pnp_string with
> kfree(old) + kstrndup(new) under opts->lock only, with no opts->refcnt
> guard, so the IEEE-1284 ID string can be changed while the function is
> bound and the gadget is enumerated.
> 
> printer_func_setup() answers the host GET_DEVICE_ID class request from
> (soft)irq context and reads the string with no lock:
> 
> 	value = strlen(*dev->pnp_string);
> 	...
> 	memcpy(buf + 2, *dev->pnp_string, value);
> 
> dev->pnp_string points at &opts->pnp_string, so a concurrent configfs
> write that frees the old string races the setup read -> use-after-free.
> 
> [ ... ]
> function is not linked into a gadget.
> 
> Fixes: 24b7ba2f88e0 ("usb: gadget: function: fix dangling pnp_string in f_printer.c")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yu Zhang <yuz08559@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015084658.70918-1-yuz08559@gmail.com?part=1