Re: [PATCH net-next v1 4/4] net/mlx5: HWS, Don't free the send ring CQ buffer if destroying the CQ fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mlx5: software UAF of embedded CQ struct when firmware destroy command fails
--

commit 46458ea83b82e1a1672d9e95c03b0f4bac750b05
Author: Yuho Choi <oss.patchbox@gmail.com>

net/mlx5: HWS, Don't free the send ring CQ buffer if destroying the CQ fails

hws_send_ring_close_cq() ignores the result of mlx5_core_destroy_cq()
and frees the CQ buffer and doorbell record with mlx5_wq_destroy()
anyway. If DESTROY_CQ fails, the CQ may still exist in firmware and
retain access to that memory after it has been returned to the
allocator.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> index df3b93499eebb..21e2d0a26e88a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> @@ -1005,7 +1005,9 @@ static int hws_send_ring_open_cq(struct mlx5_core_dev *mdev,
>  
>  static void hws_send_ring_close_cq(struct mlx5hws_send_ring_cq *cq)
>  {
> -	mlx5_core_destroy_cq(cq->mdev, &cq->mcq);
> +	/* A CQ that was not destroyed can still access its buffer, leak it */
> +	if (mlx5_core_destroy_cq(cq->mdev, &cq->mcq))
> +		return;

[Severity: High]
Does returning early here expose the software CQ structure to a use-after-free?

When mlx5_core_destroy_cq() fails, it skips synchronization and completion
wait for concurrent users of the software CQ:

drivers/net/ethernet/mellanox/mlx5/core/cq.c:mlx5_core_destroy_cq() {
	...
	err = mlx5_cmd_exec_in(dev, destroy_cq, in);
	if (err)
		return err;

	synchronize_irq(cq->irqn);
	mlx5_cq_put(cq);
	wait_for_completion(&cq->free);
	...
}

Because hws_send_ring_close_cq() now returns without any indication of failure,
its caller (mlx5hws_send_queues_close()) will proceed to unconditionally free
the array embedding the CQ structure:

drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c:mlx5hws_send_queues_close() {
	...
	kfree(ctx->send_queue);
	...
}

Since wait_for_completion() and synchronize_irq() were skipped, could an
executing concurrent EQ interrupt handler or async event handler still hold
a reference to mcq and access it after the send_queue array has been freed?

>  	mlx5_wq_destroy(&cq->wq_ctrl);
>  }
>

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