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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y3dIXUmjTfJLpPe7@google.com>
Date:   Fri, 18 Nov 2022 08:54:53 +0000
From:   Lee Jones <lee@...nel.org>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Greg KH <gregkh@...uxfoundation.org>, balbi@...nel.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on
 shared f_hidg pointer

On Thu, 17 Nov 2022, Alan Stern wrote:

> On Thu, Nov 17, 2022 at 01:46:26PM +0000, Lee Jones wrote:
> > On Thu, 17 Nov 2022, Greg KH wrote:
> > 
> > > On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > > > +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> > > > +{
> > > > +	return !!kref_read(&hidg->cdev.kobj.kref);
> > > > +}
> > > 
> > > Ick, sorry, no, that's not going to work and is not allowed at all.
> > > That's some major layering violations there, AND it can change after you
> > > get the value as well.
> > 
> > This cdev belongs solely to this driver.  Hence the *.*.* and not
> > *->*->*.  What is preventing us from reading our own data?  If we
> > cannot do this directly, can I create an API to do it 'officially'?
> > 
> > I do, however, appreciate that a little locking wouldn't go amiss.
> > 
> > If this solution is not acceptable either, then we're left up the
> > creak without a paddle.  The rules you've communicated are not
> > compatible with each other.
> > 
> > Rule 1: Only one item in a data structure can reference count.
> > 
> > Due to the embedded cdev struct, this rules out my first solution of
> > giving f_hidg its own kref so that it can conduct its own life-time
> > management.
> > 
> > A potential option to satisfy this rule would be to remove the cdev
> > attribute and create its data dynamically instead.  However, the
> > staticness of cdev is used to obtain f_hidg (with container_of()) in
> > the character device handling component, so it cannot be removed.
> 
> You have not understood this rule correctly.  Only one item in a data 
> structure can hold a reference count _for that structure_.  But several 
> items in a structure can hold reference counts for themselves.

Here was the review comment I was working to on this patch [0]:

 "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."

> So for example, you could put a kref in f_hidg which would hold the 
> reference count for the f_hidg structure, while at the same time 
> including an embedded cdev with its own reference counter.  The point is 
> that the refcount in the embedded cdev refers to the lifetime of the 
> cdev, not the lifetime of the f_hidg.

This was the approach in the original submission [1], which during
review I was told was unacceptable for the aforementioned reason.

[0] https://lore.kernel.org/all/Y1PnoMvDmZMqXScw@kroah.com/
[1] https://lore.kernel.org/all/20221017112737.230772-1-lee@kernel.org/

> To make this work properly, you have to do two additional things:
> 
> 	When the cdev's refcount is initialized, increment the kref
> 	in f_hidg.
> 
> 	When the cdev's refcount drops to 0, decrement the kref (and
> 	release f_hidg if the kref hits 0).

More than happy to revisit the first solution with Greg's blessing.

-- 
Lee Jones [李琼斯]