Re: [PATCH net-next v1 2/4] net/mlx5: Don't free the WC test CQ if destroying it fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/mlx5: SQ DMA use-after-free due to dropped DESTROY_SQ error
--

commit 7355666e6a6920857efdc4ded19eec645c9bd315
Author: Yuho Choi <oss.patchbox@gmail.com>

net/mlx5: Don't free the WC test CQ if destroying it fails

This commit fixes a DMA use-after-free by returning the error from
mlx5_wc_destroy_cq() and keeping the CQ buffer and struct mlx5_wc_sq
if DESTROY_CQ fails. This prevents the hardware from retaining access
to a freed buffer.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/wc.c b/drivers/net/ethernet/mellanox/mlx5/core/wc.c
> index 7d3d4d739e2cd..01e9800ff6fd9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/wc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/wc.c

[ ... ]

> @@ -396,11 +401,13 @@ static void mlx5_core_test_wc(struct mlx5_core_dev *mdev)
>  	mlx5_wc_destroy_sq(sq);

[Severity: High]
Does this same vulnerability still exist for the SQ?

If the firmware DESTROY_SQ command fails (e.g., due to a timeout), the
hardware retains access to the SQ DMA buffer. Because
mlx5_core_destroy_sq() drops the error and returns void:

drivers/net/ethernet/mellanox/mlx5/core/transobj.c:mlx5_core_destroy_sq() {
    ...
    mlx5_cmd_exec_in(dev, destroy_sq, in);
}

The caller unconditionally frees the SQ buffer via mlx5_wq_destroy():

drivers/net/ethernet/mellanox/mlx5/core/wc.c:mlx5_wc_destroy_sq() {
    mlx5_core_destroy_sq(sq->cq.mdev, sq->sqn);
    mlx5_wq_destroy(&sq->wq_ctrl);
}

Could the hardware read garbage data from the freed memory as WQEs,
leading to arbitrary hardware DMA operations, similar to the CQ
vulnerability this patch fixes?

>  
>  err_create_sq:
> -	mlx5_wc_destroy_cq(&sq->cq);
> +	/* An undestroyed CQ may still be in use; keep the SQ that holds it. */
> +	keep_sq = mlx5_wc_destroy_cq(&sq->cq);
>  err_create_cq:
>  	mlx5_free_bfreg(mdev, &sq->bfreg);
>  err_alloc_bfreg:
> -	kfree(sq);
> +	if (!keep_sq)
> +		kfree(sq);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016045849.813696-6-oss.patchbox@gmail.com?part=2