Re: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on shared f_hidg pointer

[John Keeping]

On Mon, Nov 21, 2022 at 02:17:56PM -0500, Alan Stern wrote:
> On Mon, Nov 21, 2022 at 06:54:55PM +0000, John Keeping wrote:
> > It turns out there's already a device being created here, just not
> > associated with the structure.  Your suggestions around
> > cdev_device_add() made me spot what's going on with that so the actual
> > fix is to pull its lifetime up to match struct f_hidg.
> 
> It's not obvious from the patch what device was already being created, 
> but never mind...

The patch has:

-       device = device_create(hidg_class, NULL, dev, NULL,
-                              "%s%d", "hidg", hidg->minor);

but this device was not previously associated with the cdev (apart from
indirectly via dev_t).

> > -- >8 --
> > Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev
> > 
> > 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.
> > 
> > [Also fix refcount leak on an error path.]
> > 
> > Signed-off-by: John Keeping <john@xxxxxxxxxxxx>
> > ---
> >  drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
> >  1 file changed, 28 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> > index ca0a7d9eaa34..0b94668a3812 100644
> > --- a/drivers/usb/gadget/function/f_hid.c
> > +++ b/drivers/usb/gadget/function/f_hid.c
> 
> > @@ -999,21 +1005,12 @@ 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);
> > +	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);
> 
> This line isn't needed; cdev_device_add() does it for you because 
> hidg->dev.devt has been set.

Thanks, I'll drop this line.