linux-kernel - RE: [char-misc-next v2 5/5] mei: create dedicated device object

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: <CY5PR11MB6366B5C574859BEE3A04FB97ED41A@CY5PR11MB6366.namprd11.prod.outlook.com>
Date: Tue, 1 Jul 2025 10:55:50 +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 v2 5/5] mei: create dedicated device object

> Subject: Re: [char-misc-next v2 5/5] mei: create dedicated device object
> 
> On Mon, Jun 30, 2025 at 12:19:42PM +0300, Alexander Usyskin wrote:
> > mei_device lifetime is managed by devm procedure of parent device.
> 
> Ick, what could go wrong...
> 
> Hint, devm is not a good thing to ever use for other reference counted
> structures that can be incremented/decremented independently.  This is
> probably never going to work.
> 
> > 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.
> 
> Ah, are you trying to explain what happens today?  This isn't obvious.
> 
Will rephrase.

> > Add dedicated device object to control driver
> > private data lifetime.
> 
> But where is that device in sysfs now?
> 
> > Rename exising parent device pointer from
> > dev to parent to avoid misuse.
> 
> Why not do this rename first?
>
Ok, will split to separate patch.
 
> > Leave power management on parent device as
> > user-space is expecting it there.
> 
> Why?  That feels totally wrong.
> 
> How does sysfs look before/after this change?  Is the bus still
> addressed properly?
> 

The sysfs is like: pci device (0000:00:16.0 usually)  -> mei0
And it is unchanged with the patch.
New device is at pci device (0000:00:16.0)  -> meid.0000:00:16.0

This is from platform with patches:
user@...tform:~$ ls -l /sys/bus/pci/devices/0000\:80\:16.0/
...
drwxr-xr-x 3 root root    0 Jun 23 14:42 0000:80:16.0-05b79a6f-4628-4d7f-899d-a91514cb32ab
...
drwxr-xr-x 3 root root    0 Jun 23 14:42 0000:80:16.0-efa2aaa6-0bb6-4194-aba2-9f59a675eeba
...
drwxr-xr-x 3 root root    0 Jun 23 14:42 mei
drwxr-xr-x 3 root root    0 Jul  1 13:34 meid.0000:80:16.0
...

The runtime power management is on PCI device.
We override usual PCI runtime pm as CSME has its own power
management mechanism.

I'm looking if we can use mei class device created with
device_create_with_groups, but it is created too late in the flow to be usefull.

- - 
Thanks,
Sasha


> >
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com>
> > ---
> >  drivers/misc/mei/bus-fixup.c    |   6 +-
> >  drivers/misc/mei/bus.c          |  24 ++++---
> >  drivers/misc/mei/client.c       |  82 +++++++++++-----------
> >  drivers/misc/mei/client.h       |   6 +-
> >  drivers/misc/mei/dma-ring.c     |   8 +--
> >  drivers/misc/mei/gsc-me.c       |  13 ++--
> >  drivers/misc/mei/hbm.c          | 121 +++++++++++++++-----------------
> >  drivers/misc/mei/hw-me.c        | 101 +++++++++++++-------------
> >  drivers/misc/mei/hw-txe.c       |  62 ++++++++--------
> >  drivers/misc/mei/init.c         |  85 +++++++++++++++-------
> >  drivers/misc/mei/interrupt.c    |  45 ++++++------
> >  drivers/misc/mei/main.c         |   9 ++-
> >  drivers/misc/mei/mei_dev.h      |  11 +--
> >  drivers/misc/mei/pci-me.c       |  12 ++--
> >  drivers/misc/mei/pci-txe.c      |  10 ++-
> >  drivers/misc/mei/platform-vsc.c |  18 ++---
> >  16 files changed, 331 insertions(+), 282 deletions(-)
> >
> > diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c
> > index 90dba20b2de7..e6a1d3534663 100644
> > --- a/drivers/misc/mei/bus-fixup.c
> > +++ b/drivers/misc/mei/bus-fixup.c
> > @@ -386,7 +386,7 @@ static int mei_nfc_if_version(struct mei_cl *cl,
> >  	ret = __mei_cl_send(cl, (u8 *)&cmd, sizeof(cmd), 0,
> >  			    MEI_CL_IO_TX_BLOCKING);
> >  	if (ret < 0) {
> > -		dev_err(bus->dev, "Could not send IF version cmd ret =
> %d\n", ret);
> > +		dev_err(&bus->dev, "Could not send IF version cmd ret =
> %d\n", ret);
> >  		return ret;
> >  	}
> >
> > @@ -401,14 +401,14 @@ static int mei_nfc_if_version(struct mei_cl *cl,
> >  	bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, &vtag,
> >  				   0, 0);
> >  	if (bytes_recv < 0 || (size_t)bytes_recv < if_version_length) {
> > -		dev_err(bus->dev, "Could not read IF version ret = %d\n",
> bytes_recv);
> > +		dev_err(&bus->dev, "Could not read IF version ret = %d\n",
> bytes_recv);
> >  		ret = -EIO;
> >  		goto err;
> >  	}
> >
> >  	memcpy(ver, reply->data, sizeof(*ver));
> >
> > -	dev_info(bus->dev, "NFC MEI VERSION: IVN 0x%x Vendor ID 0x%x
> Type 0x%x\n",
> > +	dev_info(&bus->dev, "NFC MEI VERSION: IVN 0x%x Vendor ID 0x%x
> Type 0x%x\n",
> >  		 ver->fw_ivn, ver->vendor_id, ver->radio_type);
> 
> When drivers are working, they should be quiet :)
> 
> thanks,
> 
> greg k-h