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.
