linux-kernel - Re: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on shared f

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y1Y8ENck93mRY9WK@google.com>
Date:   Mon, 24 Oct 2022 08:17:36 +0100
From:   Lee Jones <lee@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on
 shared f_hidg pointer

On Sat, 22 Oct 2022, Greg Kroah-Hartman wrote:

> On Mon, Oct 17, 2022 at 12:27:37PM +0100, Lee Jones wrote:
> > A pointer to the main HID gadget struct (*f_hidg) is shared with the
> > character device handing (read() and write(), etc support) on open().
> > However external references are presently not tracked.  This could
> > easily lead to some unsavoury behaviour if gadget support is disabled
> > / disconnected.  Keeping track of the refcount ensures that resources
> > are only freed *after* they are no longer in use.
> > 
> > Cc: Felipe Balbi <balbi@...nel.org>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: linux-usb@...r.kernel.org
> > Signed-off-by: Lee Jones <lee@...nel.org>
> > ---
> >  drivers/usb/gadget/function/f_hid.c | 37 +++++++++++++++++++++--------
> >  1 file changed, 27 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> > index ca0a7d9eaa34e..79d4ee8a5647f 100644
> > --- a/drivers/usb/gadget/function/f_hid.c
> > +++ b/drivers/usb/gadget/function/f_hid.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/module.h>
> >  #include <linux/hid.h>
> >  #include <linux/idr.h>
> > +#include <linux/kref.h>
> >  #include <linux/cdev.h>
> >  #include <linux/mutex.h>
> >  #include <linux/poll.h>
> > @@ -77,6 +78,8 @@ struct f_hidg {
> >  
> >  	struct usb_ep			*in_ep;
> >  	struct usb_ep			*out_ep;
> > +
> > +	struct kref			refcount;
> >  };
> 
> While at first glance, it seems that f_hidg is not reference counted, it
> really is, with the embedded "struct cdev" a few lines above this.
> 
> That is the reference count that should control the lifecycle of this
> object, not another reference here in the "outer layer" structure.
> 
> But, the cdev api is tricky and messy and not really set up to control
> the lifecycle of objects it is embedded in.  There have been attempts in
> the past to handle this with things like the cdev_device_del() function,
> but that's not going to work here for you as you don't have a real
> struct device in f_hidg (heck, there's no device pointer in there, which
> is a different issue...)
> 
> But, you can just rip the cdev out, and make it a pointer to a cdev, and
> then you will have a better chance at getting the reference counting
> correct here.  Yes, that will be 3 different reference counted objects
> all interacting at once, but hopefully it's a bit more sane.  Try
> cleaning things up that way and allocate the cdev with a call to
> cdev_alloc() right before the cdev_init() call in this file, and then it
> might work better.

I'll look into it, thanks.

> Yeah, the cdev api is really messy, it's been on my todo list for 20+
> years now to clean it up :(
> 
> Also, one other thing, semi-related to this change:
> 
> > +static void hidg_free_resources(struct kref *ref)
> > +{
> > +	struct f_hidg *hidg = container_of(ref, struct f_hidg, refcount);
> > +	struct f_hid_opts *opts = container_of(hidg->func.fi, struct f_hid_opts, func_inst);
> > +
> > +	mutex_lock(&opts->lock);
> > +	kfree(hidg->report_desc);
> > +	kfree(hidg->set_report_buf);
> > +	kfree(hidg);
> > +	--opts->refcnt;
> 
> That's not a real reference count :(
> 
> Moving that to a kref would also be good.  Or it might just be able to
> be dropped entirely, I don't really understand what it's attempting to
> reference count at all here.

>From my, perhaps limited, understanding, refcnt is not being used as a
reference counter.  Instead it's being used as a sort of mutual
exclusion replacement?  The Ops check refcnt before doing anything
useful and if it's found to be >0, they return -EBUSY.

-- 
Lee Jones [李琼斯]