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...
> -- >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.
> + status = cdev_device_add(&hidg->cdev, &hidg->dev);
> if (status)
> goto fail_free_descs;
> @@ -1277,17 +1272,28 @@ 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);
> + }
> +
Otherwise this looks okay (although I don't know any of the details of
how fhidg works, so you shouldn't take my word for it).
Alan Stern
