Re: [PATCH v3 09/11] vfio: selftests: Add mlx5 driver - HW init and command interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Medium] Valid hardware IDs like 0 are incorrectly treated as unallocated resources, leading to resource leaks during teardown.
- [Medium] Stale command data read due to missing ownership bit poll in async command completion.
- [Medium] Asynchronous MANAGE_PAGES reclaim loses track of remaining pages if firmware claims fewer pages than requested.
- [Medium] Heap buffer overflow when processing unvalidated `num_claimed` count from firmware.
- [Medium] Out-of-bounds MMIO pointer derivation using unvalidated `uar_page` index.
- [Medium] Undefined behavior due to bitwise shift exceeding integer width.
- [Medium] Spurious failure during HCA teardown when the firmware temporarily returns 0 reclaimed pages.
- [Medium] Unconditional RoCE capability query crashes the selftest on devices where RoCE is disabled or unsupported.
--

--- Patch [9]: [PATCH v3 09/11] vfio: selftests: Add mlx5 driver - HW init and command interface ---
commit c0063c1d972772b9afac6911db9fe5ce5e21c852
Author: Jason Gunthorpe <jgg@nvidia.com>

vfio: selftests: Add mlx5 driver - HW init and command interface

This commit adds an mlx5 ConnectX selftest driver that initializes hardware
VFs and PFs through the bare-metal command interface. It registers with the
VFIO selftest framework and handles base HCA boot sequences, EQ creation,
and resource allocation.

> diff --git a/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c
[ ... ]
> +	/* Read command queue geometry from BAR */
> +	log_sz = MLX5_GET_MMIO(initial_seg, seg, log_cmdq_size);
> +	dev->cmd_log_stride = MLX5_GET_MMIO(initial_seg, seg, log_cmdq_stride);
> +	dev->pages_slot = (1 << log_sz) - 1;
> +
> +	VFIO_ASSERT_LE((unsigned int)(1 << log_sz), 32u);

[Severity: Medium]
Does this bitwise shift trigger undefined behavior if log_sz is 31 or
greater? The shift 1 << log_sz uses a 32-bit signed integer literal,
and it evaluates before the bounds check in VFIO_ASSERT_LE is reached.

[ ... ]
> +	} 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);
> +	}

[Severity: Medium]
Does this lose track of the remaining pages if the firmware claims fewer
pages than requested?

In mlx5st_pages_slot_kick, the full batch is deducted from
dev->pages_pending_reclaim before the command executes. If the completion
handler receives a num_claimed less than the batch size, the un-reclaimed
difference does not seem to be added back.

[ ... ]
> +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: Medium]
Can this result in reading stale command data?

The output data is being read here without first polling the ownership bit
of the command entry. Does the EQE delivery guarantee that the DMA write of
the output data is fully visible to the CPU, or do we still need to check
MLX5_GET_ONCE(cmd_queue_entry, cmd, ownership)?

[ ... ]
> +		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: Medium]
Could this cause a heap buffer overflow?

If the firmware returns a num_claimed value larger than the allocated batch
size of the iovas array, this loop will write out of bounds. Should we
validate num_claimed against the requested batch size before iterating?

[ ... ]
> +static void mlx5st_alloc_uar(struct mlx5st_device *dev)
> +{
> +	u32 out[MLX5_ST_SZ_DW(alloc_uar_out)] = {};
> +	u32 in[MLX5_ST_SZ_DW(alloc_uar_in)] = {};
> +
> +	MLX5_SET(alloc_uar_in, in, opcode, MLX5_CMD_OP_ALLOC_UAR);
> +	mlx5st_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
> +
> +	dev->uar_page = MLX5_GET(alloc_uar_out, out, uar);
> +	dev->uar_base = (u8 __iomem*)dev->bar0 + dev->uar_page * MLX5_HW_PAGE_SIZE;

[Severity: Medium]
Can this calculation result in an out-of-bounds pointer?

If the firmware returns an unexpectedly large uar index, dev->uar_base
could point outside the boundaries of the PCI BAR, which might cause a
segmentation fault when a doorbell is later written.

[ ... ]
> +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: Medium]
As mentioned in mlx5st_pages_slot_complete, could this also cause a heap
buffer overflow if the hardware returns a num_claimed larger than the batch
size of the iovas array?

> +
> +		mlx5st_fw_pages_free(dev, num_claimed, iovas);
> +		total_claimed += num_claimed;
> +		npages -= num_claimed;
> +
> +		free(iovas);
> +		free(in);
> +		free(out);
> +
> +		if (!num_claimed && !dev->fw_pages_given)
> +			break;
> +		if (!num_claimed)
> +			VFIO_FAIL("MANAGE_PAGES RECLAIM: FW returned 0 but %d pages still given",
> +				  dev->fw_pages_given);

[Severity: Medium]
Will this abort the selftest sporadically?

The hardware can temporarily return 0 reclaimed pages while flushing internal
states before making the pages available. Should this implement a sleep-retry
loop instead of immediately asserting failure?

[ ... ]
> +static void mlx5st_hca_teardown(struct mlx5st_device *dev)
> +{
[ ... ]
> +	dev->have_eq = false;
> +
> +	if (dev->eqn) {
> +		mlx5st_destroy_eq(dev);
> +		dev->eqn = 0;
> +	}

[Severity: Medium]
Could this skip teardown if the hardware allocates an EQ with an ID of 0?

Since 0 can be a valid hardware allocation ID, checking if (dev->eqn)
might skip destruction and leak the resource. Would it be better to check
dev->have_eq here?

[ ... ]
> +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);
> +
> +	mlx5st_cmd_exec(dev, in, sizeof(in), out, sizeof(out));

[Severity: Medium]
Could this crash the selftest on devices that lack RoCE support?

Issuing a QUERY_HCA_CAP for the RoCE capability page blindly might
result in a BAD_PARAM failure from firmware if RoCE is disabled. Should we
check the roce support bit in GENERAL_DEVICE capabilities first?

[ ... ]
> +static void mlx5st_remove(struct vfio_pci_device *device)
> +{
> +	struct mlx5st_device *dev = to_mlx5st(device);
> +
> +	dev_dbg(device, "teardown: destroy_mkey\n");
> +	if (dev->mkey_index) {
> +		mlx5st_destroy_mkey(dev);
> +		dev->mkey_index = 0;
> +	}
> +
> +	dev_dbg(device, "teardown: dealloc_pd\n");
> +	if (dev->pdn) {
> +		mlx5st_dealloc_pd(dev);
> +		dev->pdn = 0;
> +	}

[Severity: Medium]
Similar to the EQ ID above, could this leak the Protection Domain if the
hardware allocates a PD with an ID of 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0-v3-76f117ad04f1+28a90-mlx5st_jgg@nvidia.com?part=9