* [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev
@ 2022-11-22 12:35 John Keeping
2022-11-22 12:35 ` [PATCH 1/3] " John Keeping
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: John Keeping @ 2022-11-22 12:35 UTC (permalink / raw)
To: linux-usb
Cc: Fabien Chouteau, Peter Korsgaard, Felipe Balbi,
Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman,
John Keeping, Lee Jones, Alan Stern
This series arises from the recent thread [1] on lifetime issues.
The main point is the first patch, with the second being an unrelated
fix for an issue spotted while working on this. Both of these have
Fixes: tags for backporting to stable.
The final patch tidies up some error handling to hopefully avoid patch 2
issues in the future.
[1] https://lore.kernel.org/r/20221117120813.1257583-1-lee@kernel.org
John Keeping (3):
usb: gadget: f_hid: fix f_hidg lifetime vs cdev
usb: gadget: f_hid: fix refcount leak on error path
usb: gadget: f_hid: tidy error handling in hidg_alloc
drivers/usb/gadget/function/f_hid.c | 60 ++++++++++++++++-------------
1 file changed, 33 insertions(+), 27 deletions(-)
--
2.38.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-22 12:35 [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev John Keeping @ 2022-11-22 12:35 ` John Keeping 2022-11-23 11:52 ` Andrzej Pietrasiewicz 2022-11-22 12:35 ` [PATCH 2/3] usb: gadget: f_hid: fix refcount leak on error path John Keeping ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: John Keeping @ 2022-11-22 12:35 UTC (permalink / raw) To: linux-usb Cc: Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, John Keeping, Lee Jones, Alan Stern The embedded struct cdev does not have its lifetime correctly tied to the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN is held open while the gadget is deleted. This can readily be replicated with libusbgx's example programs (for conciseness - operating directly via configfs is equivalent): gadget-hid exec 3<> /dev/hidg0 gadget-vid-pid-remove exec 3<&- Pull the existing device up in to struct f_hidg and make use of the cdev_device_{add,del}() helpers. This changes the lifetime of the device object to match struct f_hidg, but note that it is still added and deleted at the same time. Fixes: 71adf1189469 ("USB: gadget: add HID gadget driver") Signed-off-by: John Keeping <john@metanate.com> --- drivers/usb/gadget/function/f_hid.c | 52 ++++++++++++++++------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c index ca0a7d9eaa34..8b8bbeaa27cb 100644 --- a/drivers/usb/gadget/function/f_hid.c +++ b/drivers/usb/gadget/function/f_hid.c @@ -71,7 +71,7 @@ struct f_hidg { wait_queue_head_t write_queue; struct usb_request *req; - int minor; + struct device dev; struct cdev cdev; struct usb_function func; @@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f) return container_of(f, struct f_hidg, func); } +static void hidg_release(struct device *dev) +{ + struct f_hidg *hidg = container_of(dev, struct f_hidg, dev); + + kfree(hidg->set_report_buf); + kfree(hidg); +} + /*-------------------------------------------------------------------------*/ /* Static descriptors */ @@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) struct usb_ep *ep; struct f_hidg *hidg = func_to_hidg(f); struct usb_string *us; - struct device *device; int status; - dev_t dev; /* maybe allocate device-global string IDs, and patch descriptors */ us = usb_gstrings_attach(c->cdev, ct_func_strings, @@ -999,21 +1005,11 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) /* create char device */ cdev_init(&hidg->cdev, &f_hidg_fops); - dev = MKDEV(major, hidg->minor); - status = cdev_add(&hidg->cdev, dev, 1); + status = cdev_device_add(&hidg->cdev, &hidg->dev); if (status) goto fail_free_descs; - device = device_create(hidg_class, NULL, dev, NULL, - "%s%d", "hidg", hidg->minor); - if (IS_ERR(device)) { - status = PTR_ERR(device); - goto del; - } - return 0; -del: - cdev_del(&hidg->cdev); fail_free_descs: usb_free_all_descriptors(f); fail: @@ -1244,9 +1240,7 @@ static void hidg_free(struct usb_function *f) hidg = func_to_hidg(f); opts = container_of(f->fi, struct f_hid_opts, func_inst); - kfree(hidg->report_desc); - kfree(hidg->set_report_buf); - kfree(hidg); + put_device(&hidg->dev); mutex_lock(&opts->lock); --opts->refcnt; mutex_unlock(&opts->lock); @@ -1256,8 +1250,7 @@ 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); + cdev_device_del(&hidg->cdev, &hidg->dev); usb_free_all_descriptors(f); } @@ -1266,6 +1259,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) { struct f_hidg *hidg; struct f_hid_opts *opts; + int ret; /* allocate and initialize one new instance */ hidg = kzalloc(sizeof(*hidg), GFP_KERNEL); @@ -1277,17 +1271,27 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) mutex_lock(&opts->lock); ++opts->refcnt; - hidg->minor = opts->minor; + device_initialize(&hidg->dev); + hidg->dev.release = hidg_release; + hidg->dev.class = hidg_class; + hidg->dev.devt = MKDEV(major, opts->minor); + ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor); + if (ret) { + --opts->refcnt; + mutex_unlock(&opts->lock); + return ERR_PTR(ret); + } + hidg->bInterfaceSubClass = opts->subclass; hidg->bInterfaceProtocol = opts->protocol; hidg->report_length = opts->report_length; hidg->report_desc_length = opts->report_desc_length; if (opts->report_desc) { - hidg->report_desc = kmemdup(opts->report_desc, - opts->report_desc_length, - GFP_KERNEL); + hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc, + opts->report_desc_length, + GFP_KERNEL); if (!hidg->report_desc) { - kfree(hidg); + put_device(&hidg->dev); mutex_unlock(&opts->lock); return ERR_PTR(-ENOMEM); } -- 2.38.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-22 12:35 ` [PATCH 1/3] " John Keeping @ 2022-11-23 11:52 ` Andrzej Pietrasiewicz 2022-11-23 12:03 ` John Keeping 0 siblings, 1 reply; 14+ messages in thread From: Andrzej Pietrasiewicz @ 2022-11-23 11:52 UTC (permalink / raw) To: John Keeping, linux-usb Cc: Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Lee Jones, Alan Stern Hi John, W dniu 22.11.2022 o 13:35, John Keeping pisze: > The embedded struct cdev does not have its lifetime correctly tied to > the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN > is held open while the gadget is deleted. > > This can readily be replicated with libusbgx's example programs (for > conciseness - operating directly via configfs is equivalent): > > gadget-hid > exec 3<> /dev/hidg0 > gadget-vid-pid-remove > exec 3<&- > > Pull the existing device up in to struct f_hidg and make use of the > cdev_device_{add,del}() helpers. This changes the lifetime of the > device object to match struct f_hidg, but note that it is still added > and deleted at the same time. > > Fixes: 71adf1189469 ("USB: gadget: add HID gadget driver") > Signed-off-by: John Keeping <john@metanate.com> > --- > drivers/usb/gadget/function/f_hid.c | 52 ++++++++++++++++------------- > 1 file changed, 28 insertions(+), 24 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c > index ca0a7d9eaa34..8b8bbeaa27cb 100644 > --- a/drivers/usb/gadget/function/f_hid.c > +++ b/drivers/usb/gadget/function/f_hid.c > @@ -71,7 +71,7 @@ struct f_hidg { > wait_queue_head_t write_queue; > struct usb_request *req; > > - int minor; > + struct device dev; > struct cdev cdev; > struct usb_function func; > > @@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f) > return container_of(f, struct f_hidg, func); > } > > +static void hidg_release(struct device *dev) > +{ > + struct f_hidg *hidg = container_of(dev, struct f_hidg, dev); > + > + kfree(hidg->set_report_buf); > + kfree(hidg); > +} > + I assume the above is supposed to free the hidg memory as a result of put_device() and you free two things here ... > /*-------------------------------------------------------------------------*/ > /* Static descriptors */ > > @@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) > struct usb_ep *ep; > struct f_hidg *hidg = func_to_hidg(f); > struct usb_string *us; > - struct device *device; > int status; > - dev_t dev; > > /* maybe allocate device-global string IDs, and patch descriptors */ > us = usb_gstrings_attach(c->cdev, ct_func_strings, > @@ -999,21 +1005,11 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) > > /* create char device */ > cdev_init(&hidg->cdev, &f_hidg_fops); > - dev = MKDEV(major, hidg->minor); > - status = cdev_add(&hidg->cdev, dev, 1); > + status = cdev_device_add(&hidg->cdev, &hidg->dev); > if (status) > goto fail_free_descs; > > - device = device_create(hidg_class, NULL, dev, NULL, > - "%s%d", "hidg", hidg->minor); > - if (IS_ERR(device)) { > - status = PTR_ERR(device); > - goto del; > - } > - > return 0; > -del: > - cdev_del(&hidg->cdev); > fail_free_descs: > usb_free_all_descriptors(f); > fail: > @@ -1244,9 +1240,7 @@ static void hidg_free(struct usb_function *f) > > hidg = func_to_hidg(f); > opts = container_of(f->fi, struct f_hid_opts, func_inst); > - kfree(hidg->report_desc); > - kfree(hidg->set_report_buf); > - kfree(hidg); ... while here 3 things used to be freed. What happens to hidg->report_desc? Regards, Andrzej ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-23 11:52 ` Andrzej Pietrasiewicz @ 2022-11-23 12:03 ` John Keeping 2022-11-23 12:20 ` Andrzej Pietrasiewicz 0 siblings, 1 reply; 14+ messages in thread From: John Keeping @ 2022-11-23 12:03 UTC (permalink / raw) To: Andrzej Pietrasiewicz Cc: linux-usb, Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Lee Jones, Alan Stern On Wed, Nov 23, 2022 at 12:52:24PM +0100, Andrzej Pietrasiewicz wrote: > Hi John, > > W dniu 22.11.2022 o 13:35, John Keeping pisze: > > The embedded struct cdev does not have its lifetime correctly tied to > > the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN > > is held open while the gadget is deleted. > > > > This can readily be replicated with libusbgx's example programs (for > > conciseness - operating directly via configfs is equivalent): > > > > gadget-hid > > exec 3<> /dev/hidg0 > > gadget-vid-pid-remove > > exec 3<&- > > > > Pull the existing device up in to struct f_hidg and make use of the > > cdev_device_{add,del}() helpers. This changes the lifetime of the > > device object to match struct f_hidg, but note that it is still added > > and deleted at the same time. > > > > Fixes: 71adf1189469 ("USB: gadget: add HID gadget driver") > > Signed-off-by: John Keeping <john@metanate.com> > > --- > > drivers/usb/gadget/function/f_hid.c | 52 ++++++++++++++++------------- > > 1 file changed, 28 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c > > index ca0a7d9eaa34..8b8bbeaa27cb 100644 > > --- a/drivers/usb/gadget/function/f_hid.c > > +++ b/drivers/usb/gadget/function/f_hid.c > > @@ -71,7 +71,7 @@ struct f_hidg { > > wait_queue_head_t write_queue; > > struct usb_request *req; > > - int minor; > > + struct device dev; > > struct cdev cdev; > > struct usb_function func; > > @@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f) > > return container_of(f, struct f_hidg, func); > > } > > +static void hidg_release(struct device *dev) > > +{ > > + struct f_hidg *hidg = container_of(dev, struct f_hidg, dev); > > + > > + kfree(hidg->set_report_buf); > > + kfree(hidg); > > +} > > + > > I assume the above is supposed to free the hidg memory as a result of > put_device() and you free two things here ... > > > /*-------------------------------------------------------------------------*/ > > /* Static descriptors */ > > @@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) > > struct usb_ep *ep; > > struct f_hidg *hidg = func_to_hidg(f); > > struct usb_string *us; > > - struct device *device; > > int status; > > - dev_t dev; > > /* maybe allocate device-global string IDs, and patch descriptors */ > > us = usb_gstrings_attach(c->cdev, ct_func_strings, > > @@ -999,21 +1005,11 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) > > /* create char device */ > > cdev_init(&hidg->cdev, &f_hidg_fops); > > - dev = MKDEV(major, hidg->minor); > > - status = cdev_add(&hidg->cdev, dev, 1); > > + status = cdev_device_add(&hidg->cdev, &hidg->dev); > > if (status) > > goto fail_free_descs; > > - device = device_create(hidg_class, NULL, dev, NULL, > > - "%s%d", "hidg", hidg->minor); > > - if (IS_ERR(device)) { > > - status = PTR_ERR(device); > > - goto del; > > - } > > - > > return 0; > > -del: > > - cdev_del(&hidg->cdev); > > fail_free_descs: > > usb_free_all_descriptors(f); > > fail: > > @@ -1244,9 +1240,7 @@ static void hidg_free(struct usb_function *f) > > hidg = func_to_hidg(f); > > opts = container_of(f->fi, struct f_hid_opts, func_inst); > > - kfree(hidg->report_desc); > > - kfree(hidg->set_report_buf); > > - kfree(hidg); > > ... while here 3 things used to be freed. What happens to hidg->report_desc? This switched to devm to simplify error handling in hidg_alloc(). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-23 12:03 ` John Keeping @ 2022-11-23 12:20 ` Andrzej Pietrasiewicz 0 siblings, 0 replies; 14+ messages in thread From: Andrzej Pietrasiewicz @ 2022-11-23 12:20 UTC (permalink / raw) To: John Keeping Cc: linux-usb, Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Lee Jones, Alan Stern W dniu 23.11.2022 o 13:03, John Keeping pisze: > On Wed, Nov 23, 2022 at 12:52:24PM +0100, Andrzej Pietrasiewicz wrote: >> Hi John, >> >> W dniu 22.11.2022 o 13:35, John Keeping pisze: >>> The embedded struct cdev does not have its lifetime correctly tied to >>> the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN >>> is held open while the gadget is deleted. >>> >>> This can readily be replicated with libusbgx's example programs (for >>> conciseness - operating directly via configfs is equivalent): >>> >>> gadget-hid >>> exec 3<> /dev/hidg0 >>> gadget-vid-pid-remove >>> exec 3<&- >>> >>> Pull the existing device up in to struct f_hidg and make use of the >>> cdev_device_{add,del}() helpers. This changes the lifetime of the >>> device object to match struct f_hidg, but note that it is still added >>> and deleted at the same time. >>> >>> Fixes: 71adf1189469 ("USB: gadget: add HID gadget driver") >>> Signed-off-by: John Keeping <john@metanate.com> >>> --- >>> drivers/usb/gadget/function/f_hid.c | 52 ++++++++++++++++------------- >>> 1 file changed, 28 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c >>> index ca0a7d9eaa34..8b8bbeaa27cb 100644 >>> --- a/drivers/usb/gadget/function/f_hid.c >>> +++ b/drivers/usb/gadget/function/f_hid.c >>> @@ -71,7 +71,7 @@ struct f_hidg { >>> wait_queue_head_t write_queue; >>> struct usb_request *req; >>> - int minor; >>> + struct device dev; >>> struct cdev cdev; >>> struct usb_function func; >>> @@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f) >>> return container_of(f, struct f_hidg, func); >>> } >>> +static void hidg_release(struct device *dev) >>> +{ >>> + struct f_hidg *hidg = container_of(dev, struct f_hidg, dev); >>> + >>> + kfree(hidg->set_report_buf); >>> + kfree(hidg); >>> +} >>> + >> >> I assume the above is supposed to free the hidg memory as a result of >> put_device() and you free two things here ... >> >>> /*-------------------------------------------------------------------------*/ >>> /* Static descriptors */ >>> @@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) >>> struct usb_ep *ep; >>> struct f_hidg *hidg = func_to_hidg(f); >>> struct usb_string *us; >>> - struct device *device; >>> int status; >>> - dev_t dev; >>> /* maybe allocate device-global string IDs, and patch descriptors */ >>> us = usb_gstrings_attach(c->cdev, ct_func_strings, >>> @@ -999,21 +1005,11 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) >>> /* create char device */ >>> cdev_init(&hidg->cdev, &f_hidg_fops); >>> - dev = MKDEV(major, hidg->minor); >>> - status = cdev_add(&hidg->cdev, dev, 1); >>> + status = cdev_device_add(&hidg->cdev, &hidg->dev); >>> if (status) >>> goto fail_free_descs; >>> - device = device_create(hidg_class, NULL, dev, NULL, >>> - "%s%d", "hidg", hidg->minor); >>> - if (IS_ERR(device)) { >>> - status = PTR_ERR(device); >>> - goto del; >>> - } >>> - >>> return 0; >>> -del: >>> - cdev_del(&hidg->cdev); >>> fail_free_descs: >>> usb_free_all_descriptors(f); >>> fail: >>> @@ -1244,9 +1240,7 @@ static void hidg_free(struct usb_function *f) >>> hidg = func_to_hidg(f); >>> opts = container_of(f->fi, struct f_hid_opts, func_inst); >>> - kfree(hidg->report_desc); >>> - kfree(hidg->set_report_buf); >>> - kfree(hidg); >> >> ... while here 3 things used to be freed. What happens to hidg->report_desc? > > This switched to devm to simplify error handling in hidg_alloc(). Ah, right! Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] usb: gadget: f_hid: fix refcount leak on error path 2022-11-22 12:35 [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev John Keeping 2022-11-22 12:35 ` [PATCH 1/3] " John Keeping @ 2022-11-22 12:35 ` John Keeping 2022-11-23 11:55 ` Andrzej Pietrasiewicz 2022-11-22 12:35 ` [PATCH 3/3] usb: gadget: f_hid: tidy error handling in hidg_alloc John Keeping 2022-11-22 18:18 ` [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev Lee Jones 3 siblings, 1 reply; 14+ messages in thread From: John Keeping @ 2022-11-22 12:35 UTC (permalink / raw) To: linux-usb Cc: Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, John Keeping, Lee Jones, Alan Stern When failing to allocate report_desc, opts->refcnt has already been incremented so it needs to be decremented to avoid leaving the options structure permanently locked. Fixes: 21a9476a7ba8 ("usb: gadget: hid: add configfs support") Signed-off-by: John Keeping <john@metanate.com> --- drivers/usb/gadget/function/f_hid.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c index 8b8bbeaa27cb..6be6009f911e 100644 --- a/drivers/usb/gadget/function/f_hid.c +++ b/drivers/usb/gadget/function/f_hid.c @@ -1292,6 +1292,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) GFP_KERNEL); if (!hidg->report_desc) { put_device(&hidg->dev); + --opts->refcnt; mutex_unlock(&opts->lock); return ERR_PTR(-ENOMEM); } -- 2.38.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] usb: gadget: f_hid: fix refcount leak on error path 2022-11-22 12:35 ` [PATCH 2/3] usb: gadget: f_hid: fix refcount leak on error path John Keeping @ 2022-11-23 11:55 ` Andrzej Pietrasiewicz 2022-11-23 12:04 ` John Keeping 0 siblings, 1 reply; 14+ messages in thread From: Andrzej Pietrasiewicz @ 2022-11-23 11:55 UTC (permalink / raw) To: John Keeping, linux-usb Cc: Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Lee Jones, Alan Stern W dniu 22.11.2022 o 13:35, John Keeping pisze: > When failing to allocate report_desc, opts->refcnt has already been > incremented so it needs to be decremented to avoid leaving the options > structure permanently locked. > > Fixes: 21a9476a7ba8 ("usb: gadget: hid: add configfs support") > Signed-off-by: John Keeping <john@metanate.com> I'd personally place the bugfix before patches 1 and 3, but anyway Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/usb/gadget/function/f_hid.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c > index 8b8bbeaa27cb..6be6009f911e 100644 > --- a/drivers/usb/gadget/function/f_hid.c > +++ b/drivers/usb/gadget/function/f_hid.c > @@ -1292,6 +1292,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) > GFP_KERNEL); > if (!hidg->report_desc) { > put_device(&hidg->dev); > + --opts->refcnt; > mutex_unlock(&opts->lock); > return ERR_PTR(-ENOMEM); > } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] usb: gadget: f_hid: fix refcount leak on error path 2022-11-23 11:55 ` Andrzej Pietrasiewicz @ 2022-11-23 12:04 ` John Keeping 0 siblings, 0 replies; 14+ messages in thread From: John Keeping @ 2022-11-23 12:04 UTC (permalink / raw) To: Andrzej Pietrasiewicz Cc: linux-usb, Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Lee Jones, Alan Stern On Wed, Nov 23, 2022 at 12:55:14PM +0100, Andrzej Pietrasiewicz wrote: > W dniu 22.11.2022 o 13:35, John Keeping pisze: > > When failing to allocate report_desc, opts->refcnt has already been > > incremented so it needs to be decremented to avoid leaving the options > > structure permanently locked. > > > > Fixes: 21a9476a7ba8 ("usb: gadget: hid: add configfs support") > > Signed-off-by: John Keeping <john@metanate.com> > > I'd personally place the bugfix before patches 1 and 3, but anyway Patch 1 is also a bugfix, I ordered 1 & 2 based on the order of the commits in the Fixes: tags. > Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > > > --- > > drivers/usb/gadget/function/f_hid.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c > > index 8b8bbeaa27cb..6be6009f911e 100644 > > --- a/drivers/usb/gadget/function/f_hid.c > > +++ b/drivers/usb/gadget/function/f_hid.c > > @@ -1292,6 +1292,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) > > GFP_KERNEL); > > if (!hidg->report_desc) { > > put_device(&hidg->dev); > > + --opts->refcnt; > > mutex_unlock(&opts->lock); > > return ERR_PTR(-ENOMEM); > > } > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] usb: gadget: f_hid: tidy error handling in hidg_alloc 2022-11-22 12:35 [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev John Keeping 2022-11-22 12:35 ` [PATCH 1/3] " John Keeping 2022-11-22 12:35 ` [PATCH 2/3] usb: gadget: f_hid: fix refcount leak on error path John Keeping @ 2022-11-22 12:35 ` John Keeping 2022-11-23 12:19 ` Andrzej Pietrasiewicz 2022-11-22 18:18 ` [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev Lee Jones 3 siblings, 1 reply; 14+ messages in thread From: John Keeping @ 2022-11-22 12:35 UTC (permalink / raw) To: linux-usb Cc: Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, John Keeping, Lee Jones, Alan Stern Unify error handling at the end of the function, reducing the risk of missing something on one of the error paths. Moving the increment of opts->refcnt later means there is no need to decrement it on the error path and is safe as this is guarded by opts->lock which is held for this entire section. Signed-off-by: John Keeping <john@metanate.com> --- drivers/usb/gadget/function/f_hid.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c index 6be6009f911e..a8da3b4a2855 100644 --- a/drivers/usb/gadget/function/f_hid.c +++ b/drivers/usb/gadget/function/f_hid.c @@ -1269,18 +1269,14 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) opts = container_of(fi, struct f_hid_opts, func_inst); mutex_lock(&opts->lock); - ++opts->refcnt; device_initialize(&hidg->dev); hidg->dev.release = hidg_release; hidg->dev.class = hidg_class; hidg->dev.devt = MKDEV(major, opts->minor); ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor); - if (ret) { - --opts->refcnt; - mutex_unlock(&opts->lock); - return ERR_PTR(ret); - } + if (ret) + goto err_unlock; hidg->bInterfaceSubClass = opts->subclass; hidg->bInterfaceProtocol = opts->protocol; @@ -1291,14 +1287,13 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) opts->report_desc_length, GFP_KERNEL); if (!hidg->report_desc) { - put_device(&hidg->dev); - --opts->refcnt; - mutex_unlock(&opts->lock); - return ERR_PTR(-ENOMEM); + ret = -ENOMEM; + goto err_put_device; } } hidg->use_out_ep = !opts->no_out_endpoint; + ++opts->refcnt; mutex_unlock(&opts->lock); hidg->func.name = "hid"; @@ -1313,6 +1308,12 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) hidg->qlen = 4; return &hidg->func; + +err_put_device: + put_device(&hidg->dev); +err_unlock: + mutex_unlock(&opts->lock); + return ERR_PTR(ret); } DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc); -- 2.38.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] usb: gadget: f_hid: tidy error handling in hidg_alloc 2022-11-22 12:35 ` [PATCH 3/3] usb: gadget: f_hid: tidy error handling in hidg_alloc John Keeping @ 2022-11-23 12:19 ` Andrzej Pietrasiewicz 0 siblings, 0 replies; 14+ messages in thread From: Andrzej Pietrasiewicz @ 2022-11-23 12:19 UTC (permalink / raw) To: John Keeping, linux-usb Cc: Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Lee Jones, Alan Stern W dniu 22.11.2022 o 13:35, John Keeping pisze: > Unify error handling at the end of the function, reducing the risk of > missing something on one of the error paths. > > Moving the increment of opts->refcnt later means there is no need to > decrement it on the error path and is safe as this is guarded by > opts->lock which is held for this entire section. > > Signed-off-by: John Keeping <john@metanate.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/usb/gadget/function/f_hid.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c > index 6be6009f911e..a8da3b4a2855 100644 > --- a/drivers/usb/gadget/function/f_hid.c > +++ b/drivers/usb/gadget/function/f_hid.c > @@ -1269,18 +1269,14 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) > opts = container_of(fi, struct f_hid_opts, func_inst); > > mutex_lock(&opts->lock); > - ++opts->refcnt; > > device_initialize(&hidg->dev); > hidg->dev.release = hidg_release; > hidg->dev.class = hidg_class; > hidg->dev.devt = MKDEV(major, opts->minor); > ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor); > - if (ret) { > - --opts->refcnt; > - mutex_unlock(&opts->lock); > - return ERR_PTR(ret); > - } > + if (ret) > + goto err_unlock; > > hidg->bInterfaceSubClass = opts->subclass; > hidg->bInterfaceProtocol = opts->protocol; > @@ -1291,14 +1287,13 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) > opts->report_desc_length, > GFP_KERNEL); > if (!hidg->report_desc) { > - put_device(&hidg->dev); > - --opts->refcnt; > - mutex_unlock(&opts->lock); > - return ERR_PTR(-ENOMEM); > + ret = -ENOMEM; > + goto err_put_device; > } > } > hidg->use_out_ep = !opts->no_out_endpoint; > > + ++opts->refcnt; > mutex_unlock(&opts->lock); > > hidg->func.name = "hid"; > @@ -1313,6 +1308,12 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi) > hidg->qlen = 4; > > return &hidg->func; > + > +err_put_device: > + put_device(&hidg->dev); > +err_unlock: > + mutex_unlock(&opts->lock); > + return ERR_PTR(ret); > } > > DECLARE_USB_FUNCTION_INIT(hid, hidg_alloc_inst, hidg_alloc); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-22 12:35 [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev John Keeping ` (2 preceding siblings ...) 2022-11-22 12:35 ` [PATCH 3/3] usb: gadget: f_hid: tidy error handling in hidg_alloc John Keeping @ 2022-11-22 18:18 ` Lee Jones 2022-11-28 14:04 ` Lee Jones 3 siblings, 1 reply; 14+ messages in thread From: Lee Jones @ 2022-11-22 18:18 UTC (permalink / raw) To: John Keeping Cc: linux-usb, Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Alan Stern On Tue, 22 Nov 2022, John Keeping wrote: > This series arises from the recent thread [1] on lifetime issues. > > The main point is the first patch, with the second being an unrelated > fix for an issue spotted while working on this. Both of these have > Fixes: tags for backporting to stable. > > The final patch tidies up some error handling to hopefully avoid patch 2 > issues in the future. > > [1] https://lore.kernel.org/r/20221117120813.1257583-1-lee@kernel.org > > John Keeping (3): > usb: gadget: f_hid: fix f_hidg lifetime vs cdev > usb: gadget: f_hid: fix refcount leak on error path > usb: gadget: f_hid: tidy error handling in hidg_alloc > > drivers/usb/gadget/function/f_hid.c | 60 ++++++++++++++++------------- > 1 file changed, 33 insertions(+), 27 deletions(-) For the set: Reviewed-by: Lee Jones <lee@kernel.org> Tested-by: Lee Jones <lee@kernel.org> -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-22 18:18 ` [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev Lee Jones @ 2022-11-28 14:04 ` Lee Jones 2022-11-28 17:47 ` Greg Kroah-Hartman 0 siblings, 1 reply; 14+ messages in thread From: Lee Jones @ 2022-11-28 14:04 UTC (permalink / raw) To: John Keeping, Greg Kroah-Hartman Cc: linux-usb, Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Greg Kroah-Hartman, Alan Stern On Tue, 22 Nov 2022, Lee Jones wrote: > On Tue, 22 Nov 2022, John Keeping wrote: > > > This series arises from the recent thread [1] on lifetime issues. > > > > The main point is the first patch, with the second being an unrelated > > fix for an issue spotted while working on this. Both of these have > > Fixes: tags for backporting to stable. > > > > The final patch tidies up some error handling to hopefully avoid patch 2 > > issues in the future. > > > > [1] https://lore.kernel.org/r/20221117120813.1257583-1-lee@kernel.org > > > > John Keeping (3): > > usb: gadget: f_hid: fix f_hidg lifetime vs cdev > > usb: gadget: f_hid: fix refcount leak on error path > > usb: gadget: f_hid: tidy error handling in hidg_alloc > > > > drivers/usb/gadget/function/f_hid.c | 60 ++++++++++++++++------------- > > 1 file changed, 33 insertions(+), 27 deletions(-) > > For the set: > > Reviewed-by: Lee Jones <lee@kernel.org> > Tested-by: Lee Jones <lee@kernel.org> Greg, is this still on your radar? -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-28 14:04 ` Lee Jones @ 2022-11-28 17:47 ` Greg Kroah-Hartman 2022-11-29 8:59 ` Lee Jones 0 siblings, 1 reply; 14+ messages in thread From: Greg Kroah-Hartman @ 2022-11-28 17:47 UTC (permalink / raw) To: Lee Jones Cc: John Keeping, linux-usb, Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Alan Stern On Mon, Nov 28, 2022 at 02:04:13PM +0000, Lee Jones wrote: > On Tue, 22 Nov 2022, Lee Jones wrote: > > > On Tue, 22 Nov 2022, John Keeping wrote: > > > > > This series arises from the recent thread [1] on lifetime issues. > > > > > > The main point is the first patch, with the second being an unrelated > > > fix for an issue spotted while working on this. Both of these have > > > Fixes: tags for backporting to stable. > > > > > > The final patch tidies up some error handling to hopefully avoid patch 2 > > > issues in the future. > > > > > > [1] https://lore.kernel.org/r/20221117120813.1257583-1-lee@kernel.org > > > > > > John Keeping (3): > > > usb: gadget: f_hid: fix f_hidg lifetime vs cdev > > > usb: gadget: f_hid: fix refcount leak on error path > > > usb: gadget: f_hid: tidy error handling in hidg_alloc > > > > > > drivers/usb/gadget/function/f_hid.c | 60 ++++++++++++++++------------- > > > 1 file changed, 33 insertions(+), 27 deletions(-) > > > > For the set: > > > > Reviewed-by: Lee Jones <lee@kernel.org> > > Tested-by: Lee Jones <lee@kernel.org> > > Greg, is this still on your radar? Yes, let me catch up on pending patches... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev 2022-11-28 17:47 ` Greg Kroah-Hartman @ 2022-11-29 8:59 ` Lee Jones 0 siblings, 0 replies; 14+ messages in thread From: Lee Jones @ 2022-11-29 8:59 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: John Keeping, linux-usb, Fabien Chouteau, Peter Korsgaard, Felipe Balbi, Andrzej Pietrasiewicz, linux-kernel, Alan Stern On Mon, 28 Nov 2022, Greg Kroah-Hartman wrote: > On Mon, Nov 28, 2022 at 02:04:13PM +0000, Lee Jones wrote: > > On Tue, 22 Nov 2022, Lee Jones wrote: > > > > > On Tue, 22 Nov 2022, John Keeping wrote: > > > > > > > This series arises from the recent thread [1] on lifetime issues. > > > > > > > > The main point is the first patch, with the second being an unrelated > > > > fix for an issue spotted while working on this. Both of these have > > > > Fixes: tags for backporting to stable. > > > > > > > > The final patch tidies up some error handling to hopefully avoid patch 2 > > > > issues in the future. > > > > > > > > [1] https://lore.kernel.org/r/20221117120813.1257583-1-lee@kernel.org > > > > > > > > John Keeping (3): > > > > usb: gadget: f_hid: fix f_hidg lifetime vs cdev > > > > usb: gadget: f_hid: fix refcount leak on error path > > > > usb: gadget: f_hid: tidy error handling in hidg_alloc > > > > > > > > drivers/usb/gadget/function/f_hid.c | 60 ++++++++++++++++------------- > > > > 1 file changed, 33 insertions(+), 27 deletions(-) > > > > > > For the set: > > > > > > Reviewed-by: Lee Jones <lee@kernel.org> > > > Tested-by: Lee Jones <lee@kernel.org> > > > > Greg, is this still on your radar? > > Yes, let me catch up on pending patches... Perfect, thank you. -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-11-29 8:59 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-22 12:35 [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev John Keeping 2022-11-22 12:35 ` [PATCH 1/3] " John Keeping 2022-11-23 11:52 ` Andrzej Pietrasiewicz 2022-11-23 12:03 ` John Keeping 2022-11-23 12:20 ` Andrzej Pietrasiewicz 2022-11-22 12:35 ` [PATCH 2/3] usb: gadget: f_hid: fix refcount leak on error path John Keeping 2022-11-23 11:55 ` Andrzej Pietrasiewicz 2022-11-23 12:04 ` John Keeping 2022-11-22 12:35 ` [PATCH 3/3] usb: gadget: f_hid: tidy error handling in hidg_alloc John Keeping 2022-11-23 12:19 ` Andrzej Pietrasiewicz 2022-11-22 18:18 ` [PATCH 0/3] usb: gadget: f_hid: fix f_hidg lifetime vs cdev Lee Jones 2022-11-28 14:04 ` Lee Jones 2022-11-28 17:47 ` Greg Kroah-Hartman 2022-11-29 8:59 ` Lee Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.