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] [day] [month] [year] [list]

Message-ID: <2025062107-riding-stress-48a4@gregkh>
Date: Sat, 21 Jun 2025 11:07:08 +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 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.

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?

thanks,

greg k-h