Re: [PATCH v5 08/10] vfio: selftests: Add mlx5 driver - HW init and command interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Heap buffer overflow due to unvalidated `output_num_entries` from device firmware.
- [High] Missing ownership poll in `mlx5st_pages_slot_complete` can lead to reading stale command output data.
- [Medium] Incorrect capability type encoding in QUERY_HCA_CAP causes the driver to query the wrong capabilities.
- [Medium] Left shift of a signed integer by 31 bits in `mlx5st_pages_slot_post` invokes undefined behavior.
- [Medium] Missing accounting for partially fulfilled async MANAGE_PAGES reclaim requests
--

commit c45174d111898b9c629ff1e5161b87d70fe0fb38
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Wed Sep 9 23:57:34 2026 +0000

    vfio: selftests: Add mlx5 driver - HW init and command interface
    
    This commit adds an mlx5 selftest driver to program ConnectX devices
    through the bare-metal command interface. It sets up initialization paths
    including EQ creation, mailbox commands, and physical page allocation for
    the hardware.

> diff --git a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> new file mode 100644
> index 0000000000000..804801cc564e7
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> @@ -0,0 +1,1414 @@
> [ ... ]
> +static void mlx5st_pages_slot_post(struct mlx5st_device *dev, void *in,
> +				    unsigned int ilen, unsigned int olen)
> +{
> +	mlx5st_cmd_post(dev, dev->pages_cmd_lay, dev->pages_in_mbox,
> +			dev->pages_out_mbox, in, ilen, olen,
> +			1 << dev->pages_slot);
> +}

[Severity: Medium]
Since dev->pages_slot can safely be 31 if the hardware exposes a command
queue size of 32, does shifting the signed integer literal 1 by 31 bits
invoke undefined behavior here? Should this be 1U << dev->pages_slot?

> [ ... ]
> +static void mlx5st_pages_slot_kick(struct mlx5st_device *dev)
> +{
> [ ... ]
> +	} else if (dev->pages_pending_reclaim) {
> +		batch = dev->pages_pending_reclaim < MAX_FW_PAGES_PER_CMD ?
> +				dev->pages_pending_reclaim :
> +				MAX_FW_PAGES_PER_CMD;
> +		dev->pages_pending_reclaim -= batch;
> +		mlx5st_pages_slot_reclaim(dev, dev->pages_pending_func_id,
> +					  batch);
> +	}
> +}
> [ ... ]
> +static void mlx5st_pages_slot_complete(struct mlx5st_device *dev)
> +{
> +	struct mlx5st_cmd_queue_entry *cmd = dev->pages_cmd_lay;
> +	void *cout;
> +
> +	dma_rmb();
> +
> +	cout = MLX5_ADDR_OF(cmd_queue_entry, cmd, command_output_inline_data);

[Severity: High]
Is there a missing ownership poll loop for the command queue entry before
consuming its output?

When mlx5st_process_events() receives an EQE for the async pages slot, it
directly calls mlx5st_pages_slot_complete(). Because of PCIe relaxed ordering,
the EQE completion DMA can become visible to the CPU before the command queue
output DMA is fully written to memory and the ownership bit is cleared. Can
the driver read stale command output data if it does not poll the ownership
bit?

> +	if (MLX5_GET(enable_hca_out, cout, status) != MLX5_CMD_STAT_OK)
> [ ... ]
> +	if (dev->pages_slot_is_reclaim) {
> +		unsigned int outlen = MLX5_ST_SZ_BYTES(manage_pages_out) +
> +				      dev->pages_reclaim_npages * 8;
> [ ... ]
> +		num_claimed =
> +			MLX5_GET(manage_pages_out, out, output_num_entries);
> +		for (i = 0; i < num_claimed; i++)
> +			iovas[i] = MLX5_ARRAY_GET64(manage_pages_out, out, pas,
> +						    i);

[Severity: High]
Does this code write past the bounds of the iovas array if the device
firmware returns a num_claimed value larger than the requested size?

The iovas array is allocated based on the requested batch size. Without
validating the hardware-supplied output_num_entries against the array bounds,
a misbehaving device might overflow this heap buffer.

[Severity: Medium]
If the hardware returns fewer pages than requested (num_claimed < batch),
are the shortfall pages permanently leaked?

In mlx5st_pages_slot_kick(), dev->pages_pending_reclaim is immediately
reduced by the requested batch size before the command is sent. It looks like
the remaining unreturned pages are neither freed nor added back to the
pending reclaim count here, meaning they will never be re-requested.

> +
> +		mlx5st_fw_pages_free(dev, num_claimed, iovas);
> [ ... ]
> +static void mlx5st_fw_pages_reclaim(struct mlx5st_device *dev, u16 func_id)
> +{
> [ ... ]
> +		mlx5st_cmd_exec(dev, in, inlen, out, outlen);
> +
> +		num_claimed =
> +			MLX5_GET(manage_pages_out, out, output_num_entries);
> +		for (i = 0; i < num_claimed; i++)
> +			iovas[i] = MLX5_ARRAY_GET64(manage_pages_out, out, pas,
> +						    i);

[Severity: High]
Is there a similar heap buffer overflow risk here?

If the firmware provides an output_num_entries larger than the batch size
the iovas array was allocated for, this loop will write past the bounds
of the heap allocation.

> [ ... ]
> +static void mlx5st_query_fl_caps(struct mlx5st_device *dev)
> +{
> +	u32 out[MLX5_ST_SZ_DW(query_hca_cap_out)] = {};
> +	u32 in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {};
> +	bool fl_roce_en, fl_roce_dis;
> +
> +	/* Query RoCE capabilities */
> +	MLX5_SET(query_hca_cap_in, in, opcode, MLX5_CMD_OP_QUERY_HCA_CAP);
> +	MLX5_SET(query_hca_cap_in, in, op_mod,
> +		 MLX5_SET_HCA_CAP_OP_MOD_ROCE | HCA_CAP_OPMOD_GET_CUR);

[Severity: Medium]
Does the MLX5_SET_HCA_CAP_OP_MOD_ROCE constant need to be shifted left by 1?

If the constant is defined as 0x4, passing it unshifted results in an op_mod
of 5. This causes the hardware to interpret bits 15:1 as capability type 2
(MLX5_CAP_ODP) instead of RoCE, causing the driver to query the wrong
capabilities.

> +
> +	mlx5st_cmd_exec(dev, in, sizeof(in), out, sizeof(out));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0-v5-4cacb99249f6+1b4-mlx5st_jgg@nvidia.com?part=8