linux-kernel - RE: [char-misc-next 1/3] mei: refcount mei

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: <CY5PR11MB6366721138AC774A10698250ED7EA@CY5PR11MB6366.namprd11.prod.outlook.com>
Date: Sun, 22 Jun 2025 09:14:10 +0000
From: "Usyskin, Alexander" <alexander.usyskin@...el.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: "Abliyev, Reuven" <reuven.abliyev@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [char-misc-next 1/3] mei: refcount mei_device

> Subject: Re: [char-misc-next 1/3] mei: refcount mei_device
> 
> On Wed, Jun 18, 2025 at 12:54:31PM +0300, Alexander Usyskin wrote:
> > mei_device lifetime is managed by devm procedure of parent device.
> > But such memory is freed on device_del.
> > Mei_device object is used by client object that may be alive after
> > parent device is removed.
> > It may lead to use-after-free if discrete graphics driver
> > unloads mei_gsc auxiliary device while user-space holds
> > open handle to mei character device.
> >
> > Replace devm lifetime management with reference counting
> > to eliminate the use-after-free.
> 
> Overall, I like the end result, but note that if you just apply this
> patch then:
> 
> > --- a/drivers/misc/mei/mei_dev.h
> > +++ b/drivers/misc/mei/mei_dev.h
> > @@ -474,6 +474,8 @@ struct mei_dev_timeouts {
> >   * @cdev        : character device
> >   * @minor       : minor number allocated for device
> >   *
> > + * @refcnt      : struct reference count
> > + *
> >   * @write_list  : write pending list
> >   * @write_waiting_list : write completion list
> >   * @ctrl_wr_list : pending control write list
> > @@ -560,6 +562,8 @@ struct mei_device {
> >  	struct cdev cdev;
> >  	int minor;
> >
> > +	struct kref refcnt;
> > +
> >  	struct list_head write_list;
> >  	struct list_head write_waiting_list;
> >  	struct list_head ctrl_wr_list;
> 
> You now have 2 reference counts controling the lifespan of this
> structure, and it will be a mess.
> 

It is about cdev? But static cdev (like before third patch) is not refcounted.
What is the second reference counter?

> Yes, you clean it up in the last patch, so overall it's ok, this is just
> a worrying step.
> 
> Also, why are you using a kref?  Why not use the real struct device if
> you want to have a reference counted device structure?  That is what
> should be happening here, what's wrong with the struct device * that you
> already have?  Why not have that take over ownership instead of making a
> newer intermediate reference counted object?
> 

The device *dev in mei_device is the parent device.
On auxiliary bus it can be removed while driver is still active.
The device_del removes all memory allocated with devm_*.
I can't find a way how to keep driver private memory alive
till last reference to parent device still held.
This why I've resorted to manual kref.
I'm feeling that I'm missing something here.
Is there any other way to do it?

- - 
Thanks,
Sasha


> thanks,
> 
> greg k-h