Re: [PATCH] misc: mei: prevent reusing disconnecting clients in mei_ioctl_connect_vtag

[gregkh@xxxxxxxxxxxxxxxxxxx]

On Fri, Sep 04, 2026 at 03:59:58AM +0000, nirbhayykumarr@xxxxxxxxx wrote:
> This issue was discovered using a custom multi-threaded C fuzzer
> designed to stress-test MEI Virtual Tag (vtag) client lifecycles and
> multiplexing over /dev/mei0. By concurrently racing rapid vtag
> connections against file descriptor closures and streaming I/O, a
> race condition is triggered during client teardown.
> 
> In mei_release(), closing the last file descriptor holding a virtual tag
> invokes mei_cl_disconnect(). Inside __mei_cl_disconnect(),
> dev->device_lock is dropped while awaiting the firmware disconnect ACK
> on cl->wait.
> 
> During this lock-drop window, a concurrent IOCTL_MEI_CONNECT_CLIENT_VTAG
> call on the same UUID scans dev->file_list. Because
> mei_ioctl_connect_vtag() only verified pos->me_cl without checking
> pos->state, it matched the tearing-down client (in
> MEI_FILE_DISCONNECTING), repointed file->private_data to pos, and added a
> new vtag to pos->vtag_map.
> 
> When the disconnect ACK arrived, __mei_cl_disconnect() called
> mei_cl_set_disconnected(pos), setting pos->me_cl = NULL and pos->state =
> MEI_FILE_DISCONNECTED. Because pos->vtag_map now contained the second
> thread's tag, mei_release() skipped unlinking and freeing pos. The second
> thread then attempted to reuse this disconnected client, causing packet
> demuxing mismatches, continuous CSME hardware link resets, and DRM/i915
> display freezes.
> 
> Fix this by:
> 1. Validating pos->state in mei_ioctl_connect_vtag() to ensure only
>    active clients (MEI_FILE_CONNECTED or MEI_FILE_CONNECTING) are reused.
> 2. Setting cb->vtag during callback allocation in mei_io_cb_init() via
>    mei_cl_vtag_by_fp().
> 3. Demuxing incoming read packets in mei_cl_irq_read_msg() by matching
>    vtag against cl->rd_pending rather than blindly dequeuing the head.
> 
> Fixes: f35fe5f47ed0 ("mei: add a vtag map for each client")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Nirbhay Kumar <nirbhayykumarr@xxxxxxxxx>
> ---
>  drivers/misc/mei/client.c    | 24 ++++++++++++-
>  drivers/misc/mei/client.h    |  1 +
>  drivers/misc/mei/interrupt.c | 65 +++++++++++++++++++++++-------------
>  drivers/misc/mei/main.c      | 24 +++----------
>  4 files changed, 69 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> index 5f648481024..b10c483673a 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -379,7 +379,7 @@ static struct mei_cl_cb *mei_io_cb_init(struct mei_cl *cl,
>  	cb->cl = cl;
>  	cb->buf_idx = 0;
>  	cb->fop_type = type;
> -	cb->vtag = 0;
> +	cb->vtag = mei_cl_vtag_by_fp(cl, fp);
>  	cb->ext_hdr = NULL;
>  
>  	return cb;
> @@ -1313,6 +1313,28 @@ const struct file *mei_cl_fp_by_vtag(const struct mei_cl *cl, u8 vtag)
>  	return ERR_PTR(-ENOENT);
>  }
>  
> +/**
> + * mei_cl_vtag_by_fp - obtain the vtag by file pointer
> + *
> + * @cl: host client
> + * @fp: pointer to file structure
> + *
> + * Return: vtag value on success, otherwise 0
> + */
> +u8 mei_cl_vtag_by_fp(const struct mei_cl *cl, const struct file *fp)
> +{
> +	struct mei_cl_vtag *cl_vtag;
> +
> +	if (!cl || !fp)
> +		return 0;
> +
> +	list_for_each_entry(cl_vtag, &cl->vtag_map, list)
> +		if (cl_vtag->fp == fp)
> +			return cl_vtag->vtag;

Where is the locking here and elsewhere in this patch?  WHat prevents
the list from changing while you walk it?  And what prevents this from
changing right after you get it?

thanks,

greg k-h