linux-kernel - Re: [char-misc-next v2] mei: bus: fix device leak

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: <2025062836-twentieth-kudos-1148@gregkh>
Date: Sat, 28 Jun 2025 14:10:32 +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] mei: bus: fix device leak

On Tue, Jun 24, 2025 at 02:05:20PM +0300, Alexander Usyskin wrote:
> The bus rescan function creates bus devices for all clients.
> The fixup routine is executed on all devices, unneeded
> devices are removed and fully initialized once set
> is_added flag to 1.

I don't understand why the mei bus is so special that it has to have
this type of flag, when no other bus has that for its devices.  The bus
code should know if the device has been properly added or not, if not,
then no release function can be called and the structure isn't even
viable to be used or touched at all.

So why is this needed?

> 
> If link to firmware is reset right after all devices are
> initialized, but before fixup is executed, the rescan tries
> to remove devices.
> The is_added flag is not set and the mei_cl_bus_dev_destroy
> returns prematurely.
> Allow to clean up device when is_added flag is unset to
> account for above scenario.
> 
> Fixes: 6009595a66e4 ("mei: bus: link client devices instead of host clients")
> Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com>
> ---
>  drivers/misc/mei/bus.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
> index 67176caf5416..f2e5d550c6b4 100644
> --- a/drivers/misc/mei/bus.c
> +++ b/drivers/misc/mei/bus.c
> @@ -1430,17 +1430,14 @@ static void mei_cl_bus_dev_stop(struct mei_cl_device *cldev)
>   */
>  static void mei_cl_bus_dev_destroy(struct mei_cl_device *cldev)
>  {
> -
>  	WARN_ON(!mutex_is_locked(&cldev->bus->cl_bus_lock));
>  
> -	if (!cldev->is_added)
> -		return;
> -
> -	device_del(&cldev->dev);
> +	if (cldev->is_added) {
> +		device_del(&cldev->dev);
> +		cldev->is_added = 0;
> +	}

How can destroy be called here if the device has not been added before?
How can it be hanging around in memory at all if the device_add() call
was not successful when it was originally called?

confused,

greg k-h