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: <Y3Y7MlwV0UFcA3w8@google.com>
Date:   Thu, 17 Nov 2022 13:46:26 +0000
From:   Lee Jones <lee@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     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, 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.

Rule 2: Reading the present refcount causes a laying violation

So we're essentially saying, if data is already being reference
counted, you have to use the present implementation instead of adding
additional counts, but there is no way to actually do that, which
kind of puts us at stalemate.

Since this is a genuine issue, doing anything is not really an option
either.  So where do we go from here?

-- 
Lee Jones [李琼斯]