linux-kernel - Re: [PATCH v6 2/2] platform/chrome: cros_ec_chardev: Consume cros_ec

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: <20251126153320.GI520526@nvidia.com>
Date: Wed, 26 Nov 2025 11:33:20 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Tzung-Bi Shih <tzungbi@...nel.org>
Cc: Benson Leung <bleung@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	chrome-platform@...ts.linux.dev, linux-kselftest@...r.kernel.org,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Simona Vetter <simona.vetter@...ll.ch>,
	Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v6 2/2] platform/chrome: cros_ec_chardev: Consume
 cros_ec_device via revocable

On Wed, Nov 26, 2025 at 04:16:29AM +0000, Tzung-Bi Shih wrote:
> On Thu, Nov 06, 2025 at 11:59:51AM -0400, Jason Gunthorpe wrote:
> > On Thu, Nov 06, 2025 at 11:26:02PM +0800, Tzung-Bi Shih wrote:
> > > @@ -166,7 +181,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
> > >  	if (!priv)
> > >  		return -ENOMEM;
> > >  
> > > -	priv->ec_dev = ec_dev;
> > > +	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
> > > +	if (!priv->ec_dev_rev) {
> > > +		ret = -ENOMEM;
> > > +		goto free_priv;
> > > +	}
> > 
> > The lifecyle of ec_dev->ec_dev->revocable_provider memory is
> > controlled by dev:
> > 
> > +       ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
> > 
> > Under the lifecycle of some other driver.
> > 
> > The above only works because misc calls open under the misc_mtx so it
> > open has "sync" behavior during misc_unregister, and other rules
> 
> My understanding is that the file is available to be opened if and only if
> the miscdevice is registered.  

Yes, through misc_mtx.

> > ensure that ec_dev is valid during the full lifecycle of this driver.
> 
> To clarify, ec_dev is only required to be valid during the .open() call
> itself, not for the entire lifecycle of the driver.  Since ec_dev can
> become invalid at any other time, the driver uses ec_dev_rev to ensure
> safe access.

open can be called during the entire lifecycle of the driver,
misc_deregister() is called during remove. So this is a meaningless
distinction.

ec_dev cannot become invalid while the driver is bound.

> > So, I think this cross-driver design an abusive use of the revocable
> > idea.
> > 
> > It should not be allocated by the parent driver, it should be fully
> > contained to this driver alone and used only to synchronize the
> > fops. This would make it clear that the ec_dev pointer must be valid
>   ^^^^
> ec_dev_rev serves this purpose, not revocable_provider.

How does this detail matter? It is still created by the wrong driver.

> > What you have here by putting the providing in another driver is too
> > magic and obfuscates what the actual lifetime rules are while
> > providing a giant foot gun for someone to think that just because it
> > is marked revocable it is fully safe to touch revocable_provider at
> > any time.
> > 
> > Broadly I think embedding a revocable in the memory that it is trying
> > to protect is probably an anti-pattern as you must somehow already
> > have a valid pointer to thing to get the revocable in the first place.
> > This severely muddies the whole notion of when it can actually be
> > revoked nor not.
> 
> ec_dev->revocable_provider should only be accessed directly within the
> .open(), as ec_dev is guaranteed to be valid there.  For all other cases,
> it uses ec_dev_rev and checks the validity with revocable_try_access()
> to determine if ec_dev has been revoked.

I understand what this does and why it works, I am saying it is an
anti-pattern bad design to cross a revocable between two drivers like
this.

You want the driver creating the fops to revoke a pointer from its own
fops - not span across multiple drivers to achieve the same thing. It
significantly confuses what the actual lifecycle rules are.

Jason