linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues
@ 2019-10-24 16:45 John Keeping
  2019-10-24 16:45 ` [PATCH 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: John Keeping @ 2019-10-24 16:45 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

File descriptors referencing the /dev/hidgN device created by the HID
gadget can outlive the underlying gadget function, which creates easy to
trigger use-after-frees in the kernel.

A simple reproduction for this using the libusbgx example programs is:

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

Closing the file descriptor on the last line triggers a use-after-free
which can be seen immediately with slub_debug=P.

This series fixes this by making the struct cdev associated with the
module rather than dynamically allocated for the gadget and changing
struct f_hidg to be refcounted instead of tied to the gadget lifetime.

John Keeping (6):
  USB: gadget: f_hid: move chardev setup to module init
  USB: gadget: f_hid: switch to IDR for tracking minors
  USB: gadget: f_hid: find f_hidg by IDR lookup on open
  USB: gadget: f_hid: decouple cdev from f_hidg lifetime
  USB: gadget: f_hid: refcount f_hidg structure
  USB: gadget: f_hid: return ENODEV from read/write after deletion

 drivers/usb/gadget/function/f_hid.c | 141 +++++++++++++++++++---------
 drivers/usb/gadget/function/u_hid.h |   3 -
 2 files changed, 95 insertions(+), 49 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/6] USB: gadget: f_hid: move chardev setup to module init
  2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
@ 2019-10-24 16:45 ` John Keeping
  2019-10-27 17:13   ` kbuild test robot
  2019-10-24 16:45 ` [PATCH 2/6] USB: gadget: f_hid: switch to IDR for tracking minors John Keeping
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: John Keeping @ 2019-10-24 16:45 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

Lifetime of the objects in the HID gadget is currently tied to the USB
function, which makes it very easy to trigger a use-after-free by
holding a file descriptor on the HID device after deleting the gadget.

As a first step towards fixing this, move the character device
allocation to module initialisation so that we don't have to worry about
when the allocation can be released after closing all of the open
handles on the device.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 27 +++++++--------------------
 drivers/usb/gadget/function/u_hid.h |  3 ---
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index f3816a5c861e..e9e45f9eae80 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -1006,11 +1006,7 @@ static void hidg_free_inst(struct usb_function_instance *f)
 	opts = container_of(f, struct f_hid_opts, func_inst);
 
 	mutex_lock(&hidg_ida_lock);
-
 	hidg_put_minor(opts->minor);
-	if (ida_is_empty(&hidg_ida))
-		ghid_cleanup();
-
 	mutex_unlock(&hidg_ida_lock);
 
 	if (opts->report_desc_alloc)
@@ -1023,7 +1019,6 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 {
 	struct f_hid_opts *opts;
 	struct usb_function_instance *ret;
-	int status = 0;
 
 	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
 	if (!opts)
@@ -1034,21 +1029,10 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 
 	mutex_lock(&hidg_ida_lock);
 
-	if (ida_is_empty(&hidg_ida)) {
-		status = ghid_setup(NULL, HIDG_MINORS);
-		if (status)  {
-			ret = ERR_PTR(status);
-			kfree(opts);
-			goto unlock;
-		}
-	}
-
 	opts->minor = hidg_get_minor();
 	if (opts->minor < 0) {
 		ret = ERR_PTR(opts->minor);
 		kfree(opts);
-		if (ida_is_empty(&hidg_ida))
-			ghid_cleanup();
 		goto unlock;
 	}
 	config_group_init_type_name(&opts->func_inst.group, "", &hid_func_type);
@@ -1133,7 +1117,7 @@ DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Fabien Chouteau");
 
-int ghid_setup(struct usb_gadget *g, int count)
+static int ghid_setup(void)
 {
 	int status;
 	dev_t dev;
@@ -1145,7 +1129,7 @@ int ghid_setup(struct usb_gadget *g, int count)
 		return status;
 	}
 
-	status = alloc_chrdev_region(&dev, 0, count, "hidg");
+	status = alloc_chrdev_region(&dev, 0, HIDG_MINORS, "hidg");
 	if (status) {
 		class_destroy(hidg_class);
 		hidg_class = NULL;
@@ -1153,12 +1137,12 @@ int ghid_setup(struct usb_gadget *g, int count)
 	}
 
 	major = MAJOR(dev);
-	minors = count;
+	minors = HIDG_MINORS;
 
 	return 0;
 }
 
-void ghid_cleanup(void)
+static void ghid_cleanup(void)
 {
 	if (major) {
 		unregister_chrdev_region(MKDEV(major, 0), minors);
@@ -1168,3 +1152,6 @@ void ghid_cleanup(void)
 	class_destroy(hidg_class);
 	hidg_class = NULL;
 }
+
+module_init(ghid_setup);
+module_exit(ghid_cleanup);
diff --git a/drivers/usb/gadget/function/u_hid.h b/drivers/usb/gadget/function/u_hid.h
index 1594bfa312eb..d52acc977c7e 100644
--- a/drivers/usb/gadget/function/u_hid.h
+++ b/drivers/usb/gadget/function/u_hid.h
@@ -33,7 +33,4 @@ struct f_hid_opts {
 	 int				refcnt;
 };
 
-int ghid_setup(struct usb_gadget *g, int count);
-void ghid_cleanup(void);
-
 #endif /* U_HID_H */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/6] USB: gadget: f_hid: switch to IDR for tracking minors
  2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
  2019-10-24 16:45 ` [PATCH 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
@ 2019-10-24 16:45 ` John Keeping
  2019-10-24 16:45 ` [PATCH 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open John Keeping
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-24 16:45 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

In the following commits, we are going to move the cdev allocation up to
the module initialisation, and will look up the associated f_hidg
structure from the open operation.

Start preparing for this by switching from IDA to IDR.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index e9e45f9eae80..838256086bd2 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -24,8 +24,8 @@
 
 static int major, minors;
 static struct class *hidg_class;
-static DEFINE_IDA(hidg_ida);
-static DEFINE_MUTEX(hidg_ida_lock); /* protects access to hidg_ida */
+static DEFINE_IDR(hidg_idr);
+static DEFINE_MUTEX(hidg_idr_lock); /* protects access to hidg_idr */
 
 /*-------------------------------------------------------------------------*/
 /*                            HID gadget struct                            */
@@ -851,9 +851,9 @@ static inline int hidg_get_minor(void)
 {
 	int ret;
 
-	ret = ida_simple_get(&hidg_ida, 0, 0, GFP_KERNEL);
+	ret = idr_alloc(&hidg_idr, NULL, 0, 0, GFP_KERNEL);
 	if (ret >= HIDG_MINORS) {
-		ida_simple_remove(&hidg_ida, ret);
+		idr_remove(&hidg_idr, ret);
 		ret = -ENODEV;
 	}
 
@@ -996,7 +996,7 @@ static const struct config_item_type hid_func_type = {
 
 static inline void hidg_put_minor(int minor)
 {
-	ida_simple_remove(&hidg_ida, minor);
+	idr_remove(&hidg_idr, minor);
 }
 
 static void hidg_free_inst(struct usb_function_instance *f)
@@ -1005,9 +1005,9 @@ static void hidg_free_inst(struct usb_function_instance *f)
 
 	opts = container_of(f, struct f_hid_opts, func_inst);
 
-	mutex_lock(&hidg_ida_lock);
+	mutex_lock(&hidg_idr_lock);
 	hidg_put_minor(opts->minor);
-	mutex_unlock(&hidg_ida_lock);
+	mutex_unlock(&hidg_idr_lock);
 
 	if (opts->report_desc_alloc)
 		kfree(opts->report_desc);
@@ -1027,7 +1027,7 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 	opts->func_inst.free_func_inst = hidg_free_inst;
 	ret = &opts->func_inst;
 
-	mutex_lock(&hidg_ida_lock);
+	mutex_lock(&hidg_idr_lock);
 
 	opts->minor = hidg_get_minor();
 	if (opts->minor < 0) {
@@ -1038,7 +1038,7 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 	config_group_init_type_name(&opts->func_inst.group, "", &hid_func_type);
 
 unlock:
-	mutex_unlock(&hidg_ida_lock);
+	mutex_unlock(&hidg_idr_lock);
 	return ret;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open
  2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
  2019-10-24 16:45 ` [PATCH 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
  2019-10-24 16:45 ` [PATCH 2/6] USB: gadget: f_hid: switch to IDR for tracking minors John Keeping
@ 2019-10-24 16:45 ` John Keeping
  2019-10-24 16:45 ` [PATCH 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime John Keeping
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-24 16:45 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

As a step towards decoupling the cdev lifetime from f_hidg, lookup the
f_hidg structure by minor number from IDR when opening the device.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 838256086bd2..4d20347fe908 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -441,8 +441,14 @@ static int f_hidg_release(struct inode *inode, struct file *fd)
 
 static int f_hidg_open(struct inode *inode, struct file *fd)
 {
-	struct f_hidg *hidg =
-		container_of(inode->i_cdev, struct f_hidg, cdev);
+	struct f_hidg *hidg;
+
+	mutex_lock(&hidg_idr_lock);
+	hidg = idr_find(&hidg_idr, iminor(inode));
+	mutex_unlock(&hidg_idr_lock);
+
+	if (!hidg)
+		return -ENODEV;
 
 	fd->private_data = hidg;
 
@@ -827,6 +833,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	if (status)
 		goto fail_free_descs;
 
+	mutex_lock(&hidg_idr_lock);
+	idr_replace(&hidg_idr, hidg, hidg->minor);
+	mutex_unlock(&hidg_idr_lock);
+
 	device = device_create(hidg_class, NULL, dev, NULL,
 			       "%s%d", "hidg", hidg->minor);
 	if (IS_ERR(device)) {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime
  2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
                   ` (2 preceding siblings ...)
  2019-10-24 16:45 ` [PATCH 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open John Keeping
@ 2019-10-24 16:45 ` John Keeping
  2019-10-24 16:45 ` [PATCH 5/6] USB: gadget: f_hid: refcount f_hidg structure John Keeping
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-24 16:45 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

The character device needs to live until the last file descriptor has
been closed (and until after ->release() is called in that case).
Change the lifetime of our character devices to match the module, so as
to avoid a use-after-free when closing a file descriptor after the
gadget function has been deleted.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 45 ++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 4d20347fe908..ee94348b85ef 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -24,6 +24,7 @@
 
 static int major, minors;
 static struct class *hidg_class;
+static struct cdev *hidg_cdev;
 static DEFINE_IDR(hidg_idr);
 static DEFINE_MUTEX(hidg_idr_lock); /* protects access to hidg_idr */
 
@@ -58,7 +59,6 @@ struct f_hidg {
 	struct usb_request		*req;
 
 	int				minor;
-	struct cdev			cdev;
 	struct usb_function		func;
 
 	struct usb_ep			*in_ep;
@@ -827,11 +827,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	INIT_LIST_HEAD(&hidg->completed_out_req);
 
 	/* create char device */
-	cdev_init(&hidg->cdev, &f_hidg_fops);
 	dev = MKDEV(major, hidg->minor);
-	status = cdev_add(&hidg->cdev, dev, 1);
-	if (status)
-		goto fail_free_descs;
 
 	mutex_lock(&hidg_idr_lock);
 	idr_replace(&hidg_idr, hidg, hidg->minor);
@@ -841,13 +837,14 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 			       "%s%d", "hidg", hidg->minor);
 	if (IS_ERR(device)) {
 		status = PTR_ERR(device);
-		goto del;
+		goto fail_idr_remove;
 	}
 
 	return 0;
-del:
-	cdev_del(&hidg->cdev);
-fail_free_descs:
+fail_idr_remove:
+	mutex_lock(&hidg_idr_lock);
+	idr_replace(&hidg_idr, NULL, hidg->minor);
+	mutex_unlock(&hidg_idr_lock);
 	usb_free_all_descriptors(f);
 fail:
 	ERROR(f->config->cdev, "hidg_bind FAILED\n");
@@ -1071,7 +1068,10 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 	struct f_hidg *hidg = func_to_hidg(f);
 
 	device_destroy(hidg_class, MKDEV(major, hidg->minor));
-	cdev_del(&hidg->cdev);
+
+	mutex_lock(&hidg_idr_lock);
+	idr_replace(&hidg_idr, NULL, hidg->minor);
+	mutex_unlock(&hidg_idr_lock);
 
 	usb_free_all_descriptors(f);
 }
@@ -1129,6 +1129,7 @@ MODULE_AUTHOR("Fabien Chouteau");
 
 static int ghid_setup(void)
 {
+	struct cdev *cdev = NULL;
 	int status;
 	dev_t dev;
 
@@ -1149,11 +1150,35 @@ static int ghid_setup(void)
 	major = MAJOR(dev);
 	minors = HIDG_MINORS;
 
+	status = -ENOMEM;
+	cdev = cdev_alloc();
+	if (!cdev)
+		goto fail_unregister;
+
+	cdev->owner = THIS_MODULE;
+	cdev->ops = &f_hidg_fops;
+	kobject_set_name(&cdev->kobj, "hidg");
+
+	status = cdev_add(cdev, dev, HIDG_MINORS);
+	if (status)
+		goto fail_put;
+
+	hidg_cdev = cdev;
 	return 0;
+
+fail_put:
+	kobject_put(&cdev->kobj);
+fail_unregister:
+	unregister_chrdev_region(dev, HIDG_MINORS);
+	class_destroy(hidg_class);
+	hidg_class = NULL;
+	return status;
 }
 
 static void ghid_cleanup(void)
 {
+	cdev_del(hidg_cdev);
+
 	if (major) {
 		unregister_chrdev_region(MKDEV(major, 0), minors);
 		major = minors = 0;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/6] USB: gadget: f_hid: refcount f_hidg structure
  2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
                   ` (3 preceding siblings ...)
  2019-10-24 16:45 ` [PATCH 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime John Keeping
@ 2019-10-24 16:45 ` John Keeping
  2019-10-24 16:45 ` [PATCH 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion John Keeping
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
  6 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-24 16:45 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

f_hidg is referenced by file descriptors opened on /dev/hidgN as well as
being the USB gadget function.  Since these file descriptors can be kept
alive after the gadget function has been deleted, we need to decouple
the lifetime of the f_hidg structure from the function.

Make f_hidg reference counted so that it remains alive after the gadget
function has been deleted if necessary.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ee94348b85ef..2c3e835962a5 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -58,6 +58,7 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
+	struct kref			kref;
 	int				minor;
 	struct usb_function		func;
 
@@ -70,6 +71,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 kref *kref)
+{
+	struct f_hidg *hidg = container_of(kref, struct f_hidg, kref);
+
+	kfree(hidg->report_desc);
+	kfree(hidg);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 
@@ -435,6 +444,9 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 
 static int f_hidg_release(struct inode *inode, struct file *fd)
 {
+	struct f_hidg *hidg = fd->private_data;
+
+	kref_put(&hidg->kref, hidg_release);
 	fd->private_data = NULL;
 	return 0;
 }
@@ -445,6 +457,8 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
 
 	mutex_lock(&hidg_idr_lock);
 	hidg = idr_find(&hidg_idr, iminor(inode));
+	if (hidg)
+		kref_get(&hidg->kref);
 	mutex_unlock(&hidg_idr_lock);
 
 	if (!hidg)
@@ -1056,8 +1070,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);
+	kref_put(&hidg->kref, hidg_release);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);
@@ -1109,6 +1122,8 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 
 	mutex_unlock(&opts->lock);
 
+	kref_init(&hidg->kref);
+
 	hidg->func.name    = "hid";
 	hidg->func.bind    = hidg_bind;
 	hidg->func.unbind  = hidg_unbind;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion
  2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
                   ` (4 preceding siblings ...)
  2019-10-24 16:45 ` [PATCH 5/6] USB: gadget: f_hid: refcount f_hidg structure John Keeping
@ 2019-10-24 16:45 ` John Keeping
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
  6 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-24 16:45 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

If a file descriptor to /dev/hidgN is kept open after the gadget
function has been deleted, reading or writing will block indefinitely
since no requests will ever be processed.

Add a flag to note that the function has been deleted and check this in
read & write if there is no other action to take.  When setting this
flag, also wake up any readers/writers so that they get ENODEV
immediately.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 2c3e835962a5..fdb8356e334b 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -59,6 +59,7 @@ struct f_hidg {
 	struct usb_request		*req;
 
 	struct kref			kref;
+	bool				deleted;
 	int				minor;
 	struct usb_function		func;
 
@@ -271,10 +272,14 @@ static ssize_t f_hidg_read(struct file *file, char __user *buffer,
 	/* wait for at least one buffer to complete */
 	while (!READ_COND) {
 		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+		if (READ_ONCE(hidg->deleted))
+			return -ENODEV;
+
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
-		if (wait_event_interruptible(hidg->read_queue, READ_COND))
+		if (wait_event_interruptible(hidg->read_queue,
+				READ_COND || READ_ONCE(hidg->deleted)))
 			return -ERESTARTSYS;
 
 		spin_lock_irqsave(&hidg->read_spinlock, flags);
@@ -358,11 +363,14 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	/* write queue */
 	while (!WRITE_COND) {
 		spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+		if (READ_ONCE(hidg->deleted))
+			return -ENODEV;
+
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
-		if (wait_event_interruptible_exclusive(
-				hidg->write_queue, WRITE_COND))
+		if (wait_event_interruptible_exclusive(hidg->write_queue,
+				WRITE_COND || READ_ONCE(hidg->deleted)))
 			return -ERESTARTSYS;
 
 		spin_lock_irqsave(&hidg->write_spinlock, flags);
@@ -1070,6 +1078,10 @@ static void hidg_free(struct usb_function *f)
 
 	hidg = func_to_hidg(f);
 	opts = container_of(f->fi, struct f_hid_opts, func_inst);
+	WRITE_ONCE(hidg->deleted, true);
+	wake_up(&hidg->read_queue);
+	wake_up(&hidg->write_queue);
+
 	kref_put(&hidg->kref, hidg_release);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/6] USB: gadget: f_hid: move chardev setup to module init
  2019-10-24 16:45 ` [PATCH 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
@ 2019-10-27 17:13   ` kbuild test robot
  0 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2019-10-27 17:13 UTC (permalink / raw)
  To: John Keeping
  Cc: kbuild-all, linux-usb, Felipe Balbi, Greg Kroah-Hartman,
	linux-kernel, John Keeping

[-- Attachment #1: Type: text/plain, Size: 9132 bytes --]

Hi John,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on balbi-usb/next]
[also build test ERROR on v5.4-rc4 next-20191025]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/John-Keeping/USB-gadget-f_hid-fix-lifetime-issues/20191028-001924
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from drivers/usb/gadget/function/f_hid.c:9:0:
   include/linux/module.h:131:42: error: redefinition of '__inittest'
     static inline initcall_t __maybe_unused __inittest(void)  \
                                             ^
>> drivers/usb/gadget/function/f_hid.c:1156:1: note: in expansion of macro 'module_init'
    module_init(ghid_setup);
    ^~~~~~~~~~~
   include/linux/module.h:131:42: note: previous definition of '__inittest' was here
     static inline initcall_t __maybe_unused __inittest(void)  \
                                             ^
>> include/linux/usb/composite.h:627:2: note: in expansion of macro 'module_init'
     module_init(_name ## mod_init);     \
     ^~~~~~~~~~~
>> drivers/usb/gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:133:6: error: redefinition of 'init_module'
     int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
         ^
>> drivers/usb/gadget/function/f_hid.c:1156:1: note: in expansion of macro 'module_init'
    module_init(ghid_setup);
    ^~~~~~~~~~~
   include/linux/module.h:133:6: note: previous definition of 'init_module' was here
     int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
         ^
>> include/linux/usb/composite.h:627:2: note: in expansion of macro 'module_init'
     module_init(_name ## mod_init);     \
     ^~~~~~~~~~~
>> drivers/usb/gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:137:42: error: redefinition of '__exittest'
     static inline exitcall_t __maybe_unused __exittest(void)  \
                                             ^
>> drivers/usb/gadget/function/f_hid.c:1157:1: note: in expansion of macro 'module_exit'
    module_exit(ghid_cleanup);
    ^~~~~~~~~~~
   include/linux/module.h:137:42: note: previous definition of '__exittest' was here
     static inline exitcall_t __maybe_unused __exittest(void)  \
                                             ^
>> include/linux/usb/composite.h:628:2: note: in expansion of macro 'module_exit'
     module_exit(_name ## mod_exit)
     ^~~~~~~~~~~
>> drivers/usb/gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:139:7: error: redefinition of 'cleanup_module'
     void cleanup_module(void) __copy(exitfn) __attribute__((alias(#exitfn)));
          ^
>> drivers/usb/gadget/function/f_hid.c:1157:1: note: in expansion of macro 'module_exit'
    module_exit(ghid_cleanup);
    ^~~~~~~~~~~
   include/linux/module.h:139:7: note: previous definition of 'cleanup_module' was here
     void cleanup_module(void) __copy(exitfn) __attribute__((alias(#exitfn)));
          ^
>> include/linux/usb/composite.h:628:2: note: in expansion of macro 'module_exit'
     module_exit(_name ## mod_exit)
     ^~~~~~~~~~~
>> drivers/usb/gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from drivers/usb//gadget/function/f_hid.c:9:0:
   include/linux/module.h:131:42: error: redefinition of '__inittest'
     static inline initcall_t __maybe_unused __inittest(void)  \
                                             ^
   drivers/usb//gadget/function/f_hid.c:1156:1: note: in expansion of macro 'module_init'
    module_init(ghid_setup);
    ^~~~~~~~~~~
   include/linux/module.h:131:42: note: previous definition of '__inittest' was here
     static inline initcall_t __maybe_unused __inittest(void)  \
                                             ^
>> include/linux/usb/composite.h:627:2: note: in expansion of macro 'module_init'
     module_init(_name ## mod_init);     \
     ^~~~~~~~~~~
   drivers/usb//gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:133:6: error: redefinition of 'init_module'
     int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
         ^
   drivers/usb//gadget/function/f_hid.c:1156:1: note: in expansion of macro 'module_init'
    module_init(ghid_setup);
    ^~~~~~~~~~~
   include/linux/module.h:133:6: note: previous definition of 'init_module' was here
     int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
         ^
>> include/linux/usb/composite.h:627:2: note: in expansion of macro 'module_init'
     module_init(_name ## mod_init);     \
     ^~~~~~~~~~~
   drivers/usb//gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:137:42: error: redefinition of '__exittest'
     static inline exitcall_t __maybe_unused __exittest(void)  \
                                             ^
   drivers/usb//gadget/function/f_hid.c:1157:1: note: in expansion of macro 'module_exit'
    module_exit(ghid_cleanup);
    ^~~~~~~~~~~
   include/linux/module.h:137:42: note: previous definition of '__exittest' was here
     static inline exitcall_t __maybe_unused __exittest(void)  \
                                             ^
>> include/linux/usb/composite.h:628:2: note: in expansion of macro 'module_exit'
     module_exit(_name ## mod_exit)
     ^~~~~~~~~~~
   drivers/usb//gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:139:7: error: redefinition of 'cleanup_module'
     void cleanup_module(void) __copy(exitfn) __attribute__((alias(#exitfn)));
          ^
   drivers/usb//gadget/function/f_hid.c:1157:1: note: in expansion of macro 'module_exit'
    module_exit(ghid_cleanup);
    ^~~~~~~~~~~
   include/linux/module.h:139:7: note: previous definition of 'cleanup_module' was here
     void cleanup_module(void) __copy(exitfn) __attribute__((alias(#exitfn)));
          ^
>> include/linux/usb/composite.h:628:2: note: in expansion of macro 'module_exit'
     module_exit(_name ## mod_exit)
     ^~~~~~~~~~~
   drivers/usb//gadget/function/f_hid.c:1116:1: note: in expansion of macro 'DECLARE_USB_FUNCTION_INIT'
    DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
    ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/module_init +1156 drivers/usb/gadget/function/f_hid.c

  1115	
> 1116	DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
  1117	MODULE_LICENSE("GPL");
  1118	MODULE_AUTHOR("Fabien Chouteau");
  1119	
  1120	static int ghid_setup(void)
  1121	{
  1122		int status;
  1123		dev_t dev;
  1124	
  1125		hidg_class = class_create(THIS_MODULE, "hidg");
  1126		if (IS_ERR(hidg_class)) {
  1127			status = PTR_ERR(hidg_class);
  1128			hidg_class = NULL;
  1129			return status;
  1130		}
  1131	
  1132		status = alloc_chrdev_region(&dev, 0, HIDG_MINORS, "hidg");
  1133		if (status) {
  1134			class_destroy(hidg_class);
  1135			hidg_class = NULL;
  1136			return status;
  1137		}
  1138	
  1139		major = MAJOR(dev);
  1140		minors = HIDG_MINORS;
  1141	
  1142		return 0;
  1143	}
  1144	
  1145	static void ghid_cleanup(void)
  1146	{
  1147		if (major) {
  1148			unregister_chrdev_region(MKDEV(major, 0), minors);
  1149			major = minors = 0;
  1150		}
  1151	
  1152		class_destroy(hidg_class);
  1153		hidg_class = NULL;
  1154	}
  1155	
> 1156	module_init(ghid_setup);
> 1157	module_exit(ghid_cleanup);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58684 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues
  2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
                   ` (5 preceding siblings ...)
  2019-10-24 16:45 ` [PATCH 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion John Keeping
@ 2019-10-28 11:42 ` John Keeping
  2019-10-28 11:42   ` [PATCH v2 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
                     ` (5 more replies)
  6 siblings, 6 replies; 15+ messages in thread
From: John Keeping @ 2019-10-28 11:42 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

File descriptors referencing the /dev/hidgN device created by the HID
gadget can outlive the underlying gadget function, which creates easy to
trigger use-after-frees in the kernel.

A simple reproduction for this using the libusbgx example programs is:

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

Closing the file descriptor on the last line triggers a use-after-free
which can be seen immediately with slub_debug=P.

This series fixes this by making the struct cdev associated with the
module rather than dynamically allocated for the gadget and changing
struct f_hidg to be refcounted instead of tied to the gadget lifetime.

v2:
- Fix compiling as a module

John Keeping (6):
  USB: gadget: f_hid: move chardev setup to module init
  USB: gadget: f_hid: switch to IDR for tracking minors
  USB: gadget: f_hid: find f_hidg by IDR lookup on open
  USB: gadget: f_hid: decouple cdev from f_hidg lifetime
  USB: gadget: f_hid: refcount f_hidg structure
  USB: gadget: f_hid: return ENODEV from read/write after deletion

 drivers/usb/gadget/function/f_hid.c | 150 +++++++++++++++++++---------
 drivers/usb/gadget/function/u_hid.h |   3 -
 2 files changed, 103 insertions(+), 50 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/6] USB: gadget: f_hid: move chardev setup to module init
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
@ 2019-10-28 11:42   ` John Keeping
  2019-10-28 11:42   ` [PATCH v2 2/6] USB: gadget: f_hid: switch to IDR for tracking minors John Keeping
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-28 11:42 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

Lifetime of the objects in the HID gadget is currently tied to the USB
function, which makes it very easy to trigger a use-after-free by
holding a file descriptor on the HID device after deleting the gadget.

As a first step towards fixing this, move the character device
allocation to module initialisation so that we don't have to worry about
when the allocation can be released after closing all of the open
handles on the device.

Note that we also have to move the USB function registration into the
initialisation function in order to avoid having two module_init()
functions.

Signed-off-by: John Keeping <john@metanate.com>
---
v2:
- Switch to DECLARE_USB_FUNCTION and manual function (de)registration to
  fix compiling as a module

 drivers/usb/gadget/function/f_hid.c | 41 ++++++++++++++---------------
 drivers/usb/gadget/function/u_hid.h |  3 ---
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index f3816a5c861e..e5e18d02b77a 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -1006,11 +1006,7 @@ static void hidg_free_inst(struct usb_function_instance *f)
 	opts = container_of(f, struct f_hid_opts, func_inst);
 
 	mutex_lock(&hidg_ida_lock);
-
 	hidg_put_minor(opts->minor);
-	if (ida_is_empty(&hidg_ida))
-		ghid_cleanup();
-
 	mutex_unlock(&hidg_ida_lock);
 
 	if (opts->report_desc_alloc)
@@ -1023,7 +1019,6 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 {
 	struct f_hid_opts *opts;
 	struct usb_function_instance *ret;
-	int status = 0;
 
 	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
 	if (!opts)
@@ -1034,21 +1029,10 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 
 	mutex_lock(&hidg_ida_lock);
 
-	if (ida_is_empty(&hidg_ida)) {
-		status = ghid_setup(NULL, HIDG_MINORS);
-		if (status)  {
-			ret = ERR_PTR(status);
-			kfree(opts);
-			goto unlock;
-		}
-	}
-
 	opts->minor = hidg_get_minor();
 	if (opts->minor < 0) {
 		ret = ERR_PTR(opts->minor);
 		kfree(opts);
-		if (ida_is_empty(&hidg_ida))
-			ghid_cleanup();
 		goto unlock;
 	}
 	config_group_init_type_name(&opts->func_inst.group, "", &hid_func_type);
@@ -1129,11 +1113,11 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	return &hidg->func;
 }
 
-DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc);
+DECLARE_USB_FUNCTION(hid, hidg_alloc_inst, hidg_alloc);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Fabien Chouteau");
 
-int ghid_setup(struct usb_gadget *g, int count)
+static int ghid_setup(void)
 {
 	int status;
 	dev_t dev;
@@ -1145,7 +1129,7 @@ int ghid_setup(struct usb_gadget *g, int count)
 		return status;
 	}
 
-	status = alloc_chrdev_region(&dev, 0, count, "hidg");
+	status = alloc_chrdev_region(&dev, 0, HIDG_MINORS, "hidg");
 	if (status) {
 		class_destroy(hidg_class);
 		hidg_class = NULL;
@@ -1153,13 +1137,25 @@ int ghid_setup(struct usb_gadget *g, int count)
 	}
 
 	major = MAJOR(dev);
-	minors = count;
+	minors = HIDG_MINORS;
+
+	status = usb_function_register(&hidusb_func);
+	if (status)
+		goto fail_unregister;
 
 	return 0;
+
+fail_unregister:
+	unregister_chrdev_region(dev, HIDG_MINORS);
+	class_destroy(hidg_class);
+	hidg_class = NULL;
+	return status;
 }
 
-void ghid_cleanup(void)
+static void ghid_cleanup(void)
 {
+	usb_function_unregister(&hidusb_func);
+
 	if (major) {
 		unregister_chrdev_region(MKDEV(major, 0), minors);
 		major = minors = 0;
@@ -1168,3 +1164,6 @@ void ghid_cleanup(void)
 	class_destroy(hidg_class);
 	hidg_class = NULL;
 }
+
+module_init(ghid_setup);
+module_exit(ghid_cleanup);
diff --git a/drivers/usb/gadget/function/u_hid.h b/drivers/usb/gadget/function/u_hid.h
index 1594bfa312eb..d52acc977c7e 100644
--- a/drivers/usb/gadget/function/u_hid.h
+++ b/drivers/usb/gadget/function/u_hid.h
@@ -33,7 +33,4 @@ struct f_hid_opts {
 	 int				refcnt;
 };
 
-int ghid_setup(struct usb_gadget *g, int count);
-void ghid_cleanup(void);
-
 #endif /* U_HID_H */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/6] USB: gadget: f_hid: switch to IDR for tracking minors
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
  2019-10-28 11:42   ` [PATCH v2 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
@ 2019-10-28 11:42   ` John Keeping
  2019-10-28 11:42   ` [PATCH v2 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open John Keeping
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-28 11:42 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

In the following commits, we are going to move the cdev allocation up to
the module initialisation, and will look up the associated f_hidg
structure from the open operation.

Start preparing for this by switching from IDA to IDR.

Signed-off-by: John Keeping <john@metanate.com>
---
v2:
- No changes

 drivers/usb/gadget/function/f_hid.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index e5e18d02b77a..1eb8b545e5b4 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -24,8 +24,8 @@
 
 static int major, minors;
 static struct class *hidg_class;
-static DEFINE_IDA(hidg_ida);
-static DEFINE_MUTEX(hidg_ida_lock); /* protects access to hidg_ida */
+static DEFINE_IDR(hidg_idr);
+static DEFINE_MUTEX(hidg_idr_lock); /* protects access to hidg_idr */
 
 /*-------------------------------------------------------------------------*/
 /*                            HID gadget struct                            */
@@ -851,9 +851,9 @@ static inline int hidg_get_minor(void)
 {
 	int ret;
 
-	ret = ida_simple_get(&hidg_ida, 0, 0, GFP_KERNEL);
+	ret = idr_alloc(&hidg_idr, NULL, 0, 0, GFP_KERNEL);
 	if (ret >= HIDG_MINORS) {
-		ida_simple_remove(&hidg_ida, ret);
+		idr_remove(&hidg_idr, ret);
 		ret = -ENODEV;
 	}
 
@@ -996,7 +996,7 @@ static const struct config_item_type hid_func_type = {
 
 static inline void hidg_put_minor(int minor)
 {
-	ida_simple_remove(&hidg_ida, minor);
+	idr_remove(&hidg_idr, minor);
 }
 
 static void hidg_free_inst(struct usb_function_instance *f)
@@ -1005,9 +1005,9 @@ static void hidg_free_inst(struct usb_function_instance *f)
 
 	opts = container_of(f, struct f_hid_opts, func_inst);
 
-	mutex_lock(&hidg_ida_lock);
+	mutex_lock(&hidg_idr_lock);
 	hidg_put_minor(opts->minor);
-	mutex_unlock(&hidg_ida_lock);
+	mutex_unlock(&hidg_idr_lock);
 
 	if (opts->report_desc_alloc)
 		kfree(opts->report_desc);
@@ -1027,7 +1027,7 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 	opts->func_inst.free_func_inst = hidg_free_inst;
 	ret = &opts->func_inst;
 
-	mutex_lock(&hidg_ida_lock);
+	mutex_lock(&hidg_idr_lock);
 
 	opts->minor = hidg_get_minor();
 	if (opts->minor < 0) {
@@ -1038,7 +1038,7 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 	config_group_init_type_name(&opts->func_inst.group, "", &hid_func_type);
 
 unlock:
-	mutex_unlock(&hidg_ida_lock);
+	mutex_unlock(&hidg_idr_lock);
 	return ret;
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
  2019-10-28 11:42   ` [PATCH v2 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
  2019-10-28 11:42   ` [PATCH v2 2/6] USB: gadget: f_hid: switch to IDR for tracking minors John Keeping
@ 2019-10-28 11:42   ` John Keeping
  2019-10-28 11:42   ` [PATCH v2 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime John Keeping
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-28 11:42 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

As a step towards decoupling the cdev lifetime from f_hidg, lookup the
f_hidg structure by minor number from IDR when opening the device.

Signed-off-by: John Keeping <john@metanate.com>
---
v2:
- No changes

 drivers/usb/gadget/function/f_hid.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 1eb8b545e5b4..6cf3b5b14ded 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -441,8 +441,14 @@ static int f_hidg_release(struct inode *inode, struct file *fd)
 
 static int f_hidg_open(struct inode *inode, struct file *fd)
 {
-	struct f_hidg *hidg =
-		container_of(inode->i_cdev, struct f_hidg, cdev);
+	struct f_hidg *hidg;
+
+	mutex_lock(&hidg_idr_lock);
+	hidg = idr_find(&hidg_idr, iminor(inode));
+	mutex_unlock(&hidg_idr_lock);
+
+	if (!hidg)
+		return -ENODEV;
 
 	fd->private_data = hidg;
 
@@ -827,6 +833,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	if (status)
 		goto fail_free_descs;
 
+	mutex_lock(&hidg_idr_lock);
+	idr_replace(&hidg_idr, hidg, hidg->minor);
+	mutex_unlock(&hidg_idr_lock);
+
 	device = device_create(hidg_class, NULL, dev, NULL,
 			       "%s%d", "hidg", hidg->minor);
 	if (IS_ERR(device)) {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
                     ` (2 preceding siblings ...)
  2019-10-28 11:42   ` [PATCH v2 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open John Keeping
@ 2019-10-28 11:42   ` John Keeping
  2019-10-28 11:42   ` [PATCH v2 5/6] USB: gadget: f_hid: refcount f_hidg structure John Keeping
  2019-10-28 11:42   ` [PATCH v2 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion John Keeping
  5 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-28 11:42 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

The character device needs to live until the last file descriptor has
been closed (and until after ->release() is called in that case).
Change the lifetime of our character devices to match the module, so as
to avoid a use-after-free when closing a file descriptor after the
gadget function has been deleted.

Signed-off-by: John Keeping <john@metanate.com>
---
v2:
- Updated for changes in patch 1

 drivers/usb/gadget/function/f_hid.c | 42 +++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 6cf3b5b14ded..eda4f24d2790 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -24,6 +24,7 @@
 
 static int major, minors;
 static struct class *hidg_class;
+static struct cdev *hidg_cdev;
 static DEFINE_IDR(hidg_idr);
 static DEFINE_MUTEX(hidg_idr_lock); /* protects access to hidg_idr */
 
@@ -58,7 +59,6 @@ struct f_hidg {
 	struct usb_request		*req;
 
 	int				minor;
-	struct cdev			cdev;
 	struct usb_function		func;
 
 	struct usb_ep			*in_ep;
@@ -827,11 +827,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	INIT_LIST_HEAD(&hidg->completed_out_req);
 
 	/* create char device */
-	cdev_init(&hidg->cdev, &f_hidg_fops);
 	dev = MKDEV(major, hidg->minor);
-	status = cdev_add(&hidg->cdev, dev, 1);
-	if (status)
-		goto fail_free_descs;
 
 	mutex_lock(&hidg_idr_lock);
 	idr_replace(&hidg_idr, hidg, hidg->minor);
@@ -841,13 +837,14 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 			       "%s%d", "hidg", hidg->minor);
 	if (IS_ERR(device)) {
 		status = PTR_ERR(device);
-		goto del;
+		goto fail_idr_remove;
 	}
 
 	return 0;
-del:
-	cdev_del(&hidg->cdev);
-fail_free_descs:
+fail_idr_remove:
+	mutex_lock(&hidg_idr_lock);
+	idr_replace(&hidg_idr, NULL, hidg->minor);
+	mutex_unlock(&hidg_idr_lock);
 	usb_free_all_descriptors(f);
 fail:
 	ERROR(f->config->cdev, "hidg_bind FAILED\n");
@@ -1071,7 +1068,10 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 	struct f_hidg *hidg = func_to_hidg(f);
 
 	device_destroy(hidg_class, MKDEV(major, hidg->minor));
-	cdev_del(&hidg->cdev);
+
+	mutex_lock(&hidg_idr_lock);
+	idr_replace(&hidg_idr, NULL, hidg->minor);
+	mutex_unlock(&hidg_idr_lock);
 
 	usb_free_all_descriptors(f);
 }
@@ -1129,6 +1129,7 @@ MODULE_AUTHOR("Fabien Chouteau");
 
 static int ghid_setup(void)
 {
+	struct cdev *cdev = NULL;
 	int status;
 	dev_t dev;
 
@@ -1149,12 +1150,30 @@ static int ghid_setup(void)
 	major = MAJOR(dev);
 	minors = HIDG_MINORS;
 
+	status = -ENOMEM;
+	cdev = cdev_alloc();
+	if (!cdev)
+		goto fail_unregister;
+
+	cdev->owner = THIS_MODULE;
+	cdev->ops = &f_hidg_fops;
+	kobject_set_name(&cdev->kobj, "hidg");
+
+	status = cdev_add(cdev, dev, HIDG_MINORS);
+	if (status)
+		goto fail_put;
+
 	status = usb_function_register(&hidusb_func);
 	if (status)
-		goto fail_unregister;
+		goto fail_cdev_del;
 
+	hidg_cdev = cdev;
 	return 0;
 
+fail_cdev_del:
+	cdev_del(cdev);
+fail_put:
+	kobject_put(&cdev->kobj);
 fail_unregister:
 	unregister_chrdev_region(dev, HIDG_MINORS);
 	class_destroy(hidg_class);
@@ -1165,6 +1184,7 @@ static int ghid_setup(void)
 static void ghid_cleanup(void)
 {
 	usb_function_unregister(&hidusb_func);
+	cdev_del(hidg_cdev);
 
 	if (major) {
 		unregister_chrdev_region(MKDEV(major, 0), minors);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 5/6] USB: gadget: f_hid: refcount f_hidg structure
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
                     ` (3 preceding siblings ...)
  2019-10-28 11:42   ` [PATCH v2 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime John Keeping
@ 2019-10-28 11:42   ` John Keeping
  2019-10-28 11:42   ` [PATCH v2 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion John Keeping
  5 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-28 11:42 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

f_hidg is referenced by file descriptors opened on /dev/hidgN as well as
being the USB gadget function.  Since these file descriptors can be kept
alive after the gadget function has been deleted, we need to decouple
the lifetime of the f_hidg structure from the function.

Make f_hidg reference counted so that it remains alive after the gadget
function has been deleted if necessary.

Signed-off-by: John Keeping <john@metanate.com>
---
v2:
- No changes

 drivers/usb/gadget/function/f_hid.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index eda4f24d2790..3d848f7a4cca 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -58,6 +58,7 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
+	struct kref			kref;
 	int				minor;
 	struct usb_function		func;
 
@@ -70,6 +71,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 kref *kref)
+{
+	struct f_hidg *hidg = container_of(kref, struct f_hidg, kref);
+
+	kfree(hidg->report_desc);
+	kfree(hidg);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 
@@ -435,6 +444,9 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 
 static int f_hidg_release(struct inode *inode, struct file *fd)
 {
+	struct f_hidg *hidg = fd->private_data;
+
+	kref_put(&hidg->kref, hidg_release);
 	fd->private_data = NULL;
 	return 0;
 }
@@ -445,6 +457,8 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
 
 	mutex_lock(&hidg_idr_lock);
 	hidg = idr_find(&hidg_idr, iminor(inode));
+	if (hidg)
+		kref_get(&hidg->kref);
 	mutex_unlock(&hidg_idr_lock);
 
 	if (!hidg)
@@ -1056,8 +1070,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);
+	kref_put(&hidg->kref, hidg_release);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);
@@ -1109,6 +1122,8 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 
 	mutex_unlock(&opts->lock);
 
+	kref_init(&hidg->kref);
+
 	hidg->func.name    = "hid";
 	hidg->func.bind    = hidg_bind;
 	hidg->func.unbind  = hidg_unbind;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion
  2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
                     ` (4 preceding siblings ...)
  2019-10-28 11:42   ` [PATCH v2 5/6] USB: gadget: f_hid: refcount f_hidg structure John Keeping
@ 2019-10-28 11:42   ` John Keeping
  5 siblings, 0 replies; 15+ messages in thread
From: John Keeping @ 2019-10-28 11:42 UTC (permalink / raw)
  To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, John Keeping

If a file descriptor to /dev/hidgN is kept open after the gadget
function has been deleted, reading or writing will block indefinitely
since no requests will ever be processed.

Add a flag to note that the function has been deleted and check this in
read & write if there is no other action to take.  When setting this
flag, also wake up any readers/writers so that they get ENODEV
immediately.

Signed-off-by: John Keeping <john@metanate.com>
---
v2:
- No changes

 drivers/usb/gadget/function/f_hid.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 3d848f7a4cca..a65bdf08ca54 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -59,6 +59,7 @@ struct f_hidg {
 	struct usb_request		*req;
 
 	struct kref			kref;
+	bool				deleted;
 	int				minor;
 	struct usb_function		func;
 
@@ -271,10 +272,14 @@ static ssize_t f_hidg_read(struct file *file, char __user *buffer,
 	/* wait for at least one buffer to complete */
 	while (!READ_COND) {
 		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+		if (READ_ONCE(hidg->deleted))
+			return -ENODEV;
+
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
-		if (wait_event_interruptible(hidg->read_queue, READ_COND))
+		if (wait_event_interruptible(hidg->read_queue,
+				READ_COND || READ_ONCE(hidg->deleted)))
 			return -ERESTARTSYS;
 
 		spin_lock_irqsave(&hidg->read_spinlock, flags);
@@ -358,11 +363,14 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	/* write queue */
 	while (!WRITE_COND) {
 		spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+		if (READ_ONCE(hidg->deleted))
+			return -ENODEV;
+
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
-		if (wait_event_interruptible_exclusive(
-				hidg->write_queue, WRITE_COND))
+		if (wait_event_interruptible_exclusive(hidg->write_queue,
+				WRITE_COND || READ_ONCE(hidg->deleted)))
 			return -ERESTARTSYS;
 
 		spin_lock_irqsave(&hidg->write_spinlock, flags);
@@ -1070,6 +1078,10 @@ static void hidg_free(struct usb_function *f)
 
 	hidg = func_to_hidg(f);
 	opts = container_of(f->fi, struct f_hid_opts, func_inst);
+	WRITE_ONCE(hidg->deleted, true);
+	wake_up(&hidg->read_queue);
+	wake_up(&hidg->write_queue);
+
 	kref_put(&hidg->kref, hidg_release);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-10-28 11:43 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-24 16:45 [PATCH 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
2019-10-24 16:45 ` [PATCH 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
2019-10-27 17:13   ` kbuild test robot
2019-10-24 16:45 ` [PATCH 2/6] USB: gadget: f_hid: switch to IDR for tracking minors John Keeping
2019-10-24 16:45 ` [PATCH 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open John Keeping
2019-10-24 16:45 ` [PATCH 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime John Keeping
2019-10-24 16:45 ` [PATCH 5/6] USB: gadget: f_hid: refcount f_hidg structure John Keeping
2019-10-24 16:45 ` [PATCH 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion John Keeping
2019-10-28 11:42 ` [PATCH v2 0/6] USB: gadget: f_hid: fix lifetime issues John Keeping
2019-10-28 11:42   ` [PATCH v2 1/6] USB: gadget: f_hid: move chardev setup to module init John Keeping
2019-10-28 11:42   ` [PATCH v2 2/6] USB: gadget: f_hid: switch to IDR for tracking minors John Keeping
2019-10-28 11:42   ` [PATCH v2 3/6] USB: gadget: f_hid: find f_hidg by IDR lookup on open John Keeping
2019-10-28 11:42   ` [PATCH v2 4/6] USB: gadget: f_hid: decouple cdev from f_hidg lifetime John Keeping
2019-10-28 11:42   ` [PATCH v2 5/6] USB: gadget: f_hid: refcount f_hidg structure John Keeping
2019-10-28 11:42   ` [PATCH v2 6/6] USB: gadget: f_hid: return ENODEV from read/write after deletion John Keeping

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).