Re: [PATCH 1/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi John,

W dniu 22.11.2022 o 13:35, John Keeping pisze:
The embedded struct cdev does not have its lifetime correctly tied to
the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for
conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Pull the existing device up in to struct f_hidg and make use of the
cdev_device_{add,del}() helpers.  This changes the lifetime of the
device object to match struct f_hidg, but note that it is still added
and deleted at the same time.

Fixes: 71adf1189469 ("USB: gadget: add HID gadget driver")
Signed-off-by: John Keeping <john@xxxxxxxxxxxx>
---
  drivers/usb/gadget/function/f_hid.c | 52 ++++++++++++++++-------------
  1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ca0a7d9eaa34..8b8bbeaa27cb 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -71,7 +71,7 @@ struct f_hidg {
  	wait_queue_head_t		write_queue;
  	struct usb_request		*req;
- int minor;
+	struct device			dev;
  	struct cdev			cdev;
  	struct usb_function		func;
@@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
  	return container_of(f, struct f_hidg, func);
  }
+static void hidg_release(struct device *dev)
+{
+	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
+
+	kfree(hidg->set_report_buf);
+	kfree(hidg);
+}
+

I assume the above is supposed to free the hidg memory as a result of
put_device() and you free two things here ...

  /*-------------------------------------------------------------------------*/
  /*                           Static descriptors                            */
@@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
  	struct usb_ep		*ep;
  	struct f_hidg		*hidg = func_to_hidg(f);
  	struct usb_string	*us;
-	struct device		*device;
  	int			status;
-	dev_t			dev;
/* maybe allocate device-global string IDs, and patch descriptors */
  	us = usb_gstrings_attach(c->cdev, ct_func_strings,
@@ -999,21 +1005,11 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
/* create char device */
  	cdev_init(&hidg->cdev, &f_hidg_fops);
-	dev = MKDEV(major, hidg->minor);
-	status = cdev_add(&hidg->cdev, dev, 1);
+	status = cdev_device_add(&hidg->cdev, &hidg->dev);
  	if (status)
  		goto fail_free_descs;
- device = device_create(hidg_class, NULL, dev, NULL,
-			       "%s%d", "hidg", hidg->minor);
-	if (IS_ERR(device)) {
-		status = PTR_ERR(device);
-		goto del;
-	}
-
  	return 0;
-del:
-	cdev_del(&hidg->cdev);
  fail_free_descs:
  	usb_free_all_descriptors(f);
  fail:
@@ -1244,9 +1240,7 @@ static void hidg_free(struct usb_function *f)
hidg = func_to_hidg(f);
  	opts = container_of(f->fi, struct f_hid_opts, func_inst);
-	kfree(hidg->report_desc);
-	kfree(hidg->set_report_buf);
-	kfree(hidg);

... while here 3 things used to be freed. What happens to hidg->report_desc?

Regards,

Andrzej



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux