* [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 [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 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 [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 [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 [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 [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 [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 [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 [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 [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 [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 [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 [flat|nested] 15+ messages in thread