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: <2025063045-bronchial-veggie-8288@gregkh>
Date: Mon, 30 Jun 2025 19:27:47 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Alexander Usyskin <alexander.usyskin@...el.com>
Cc: Reuven Abliyev <reuven.abliyev@...el.com>, linux-kernel@...r.kernel.org
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.

> 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?

> 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?

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