Re: [PATCH net 2/2] net/mlx5: Don't return firmware-owned command mailboxes to the DMA pool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] net/mlx5: data race on own_msgs causes DMA pool UAF
--

commit 0cea3fb4e0d198ed012c8154db5a71dcabb95879
Author: Danielle Costantino <dcostantino@meta.com>

net/mlx5: Don't return firmware-owned command mailboxes to the DMA pool

This commit transfers ownership of timed-out command mailboxes to the command
entry rather than returning them to the DMA pool. This prevents late firmware
completions from overwriting the pool's free list pointers. The mailboxes are
later leaked or freed during teardown.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 571ed540957b1..890a2a2613ca0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
[ ... ]
> @@ -207,6 +214,15 @@ static void cmd_ent_put(struct mlx5_cmd_work_ent *ent)
>  	}
>  	spin_unlock_irqrestore(&cmd->alloc_lock, flags);
>  
> +	/* These were withheld from dev->cmd.pool because firmware still owned
> +	 * them.  Nothing references the entry any more, so whoever dropped the
> +	 * last reference has established that firmware is done with them.
> +	 */
> +	if (ent->own_msgs) {
> +		mlx5_free_cmd_msg(dev, ent->out);
> +		free_msg(dev, ent->in);
> +	}
> +
>  	cmd_free_ent(ent);
>  }

[Severity: Critical]
If mlx5_cmd_invoke() and cmd_reclaim_stalled_ents() race to update
ent->own_msgs without synchronization, this block in cmd_ent_put() might
execute.

Does this risk freeing mailboxes into an already destroyed DMA pool,
leading to a use-after-free?

[ ... ]
> @@ -1313,7 +1335,19 @@ static int mlx5_cmd_invoke(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *in,
>  		return 0; /* mlx5_cmd_comp_handler() will put(ent) */
>  
>  	err = wait_func(dev, ent);
> -	if (err == -ETIMEDOUT || err == -ECANCELED || err == -EBUSY)
> +	if (err == -ETIMEDOUT) {
> +		/* The command was posted to firmware and firmware never
> +		 * completed it.  Unless the device is already down,
> +		 * mlx5_cmd_comp_handler(forced) keeps this entry and its
> +		 * queue slot allocated because firmware may still complete
> +		 * it, so ent->lay->{in_ptr,out_ptr} keep referencing the
> +		 * mailboxes.  Take ownership of them so that cmd_exec()
> +		 * cannot hand them back to dev->cmd.pool.
> +		 */
> +		ent->own_msgs = true;
> +		goto out_free;
> +	}

[Severity: Critical]
If a synchronous firmware command times out, the thread wakes up from wait_func()
and sets ent->own_msgs = true without synchronization in mlx5_cmd_invoke().

This could overwrite the ent->own_msgs = false set concurrently by the teardown
process.

Can the issuer thread clobber the safe leak configuration set during teardown?

[ ... ]
> +static void cmd_reclaim_stalled_ents(struct mlx5_core_dev *dev, bool fw_stopped)
> +{
> +	struct mlx5_cmd_work_ent *ent;
> +	struct mlx5_cmd *cmd = &dev->cmd;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < (1 << cmd->vars.log_sz); i++) {
[ ... ]
> +		/* The entry itself is never visible to firmware - the only
> +		 * addresses it is ever given are the mailbox and command
> +		 * queue DMA addresses - so it can always be retired.  Its
> +		 * mailboxes are a different matter: hand those back only once
> +		 * firmware has acknowledged it released the function.  If it
> +		 * has not, drop ownership without freeing.  dma_pool_destroy()
> +		 * then reports the pool busy and skips dma_free_coherent(),
> +		 * so the pages stay mapped and are never handed back to the
> +		 * allocator; a late write lands there harmlessly.
> +		 */
> +		if (!fw_stopped)
> +			ent->own_msgs = false;

[Severity: Critical]
Teardown correctly attempts to leak the mailboxes to prevent memory corruption by
setting ent->own_msgs = false in cmd_reclaim_stalled_ents().

However, if the issuer thread concurrently sets ent->own_msgs = true, the
mailboxes will be incorrectly freed.

Is it possible for the teardown process to be raced by the issuer thread?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915225941.554568-1-dcostantino@meta.com?part=2