Re: [PATCH v6 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 4 potential issue(s) to consider:
- [Medium] mlx5: selftest hang during teardown if async pages slot fails to clear
- [Medium] mlx5: heap buffer overflow when parsing malicious firmware reclaim responses
- [Low] mlx5: integer underflow if firmware returns duplicate pages
- [Low] mlx5: undefined behavior due to signed left shift by 31 bits
--

commit 68a628cb8bf0f80c85d9912ed7b2b29ec71b2e76
Author: Jason Gunthorpe <jgg@ziepe.ca>

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

This commit adds an mlx5 ConnectX selftest driver that programs VFs and PFs
through the command interface. It creates the driver skeleton with probe(),
the command interface, HCA boot sequence, 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
> new file mode 100644
> index 0000000000000..5a87d070dc425
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c

[ ... ]

> +static void mlx5st_fw_pages_free(struct mlx5st_device *dev,
> +				  unsigned int npages, const u64 *iovas)
> +{
> +	struct vfio_pci_device *device = dev->device;
> +	unsigned int i, idx;
> +	u64 off;
> +
> +	for (i = 0; i < npages; i++) {
> +		off = iovas[i] - to_iova(device, dev->fw_pages);
> +		idx = off / MLX5_HW_PAGE_SIZE;
> +
> +		VFIO_ASSERT_TRUE(idx < MAX_FW_PAGES);
> +		dev->fw_pages_bitmap[idx / 64] &= ~(1ULL << (idx % 64));
> +	}
> +	dev->fw_pages_given -= npages;

[Severity: Low]
Could this integer subtraction underflow if the firmware returns duplicate
pages?

The bitwise operation clears the bit without validating whether the page was
actually allocated. If the same IOVA is returned multiple times, this will
subtract from dev->fw_pages_given multiple times and may wrap around to a very
large value.

[ ... ]

> +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: Low]
Does this code trigger undefined behavior if dev->pages_slot is 31?

The shift uses a signed integer literal '1'. When shifting by 31 bits into the
sign bit, this causes undefined behavior.

Should this use '1U << dev->pages_slot' instead?

[ ... ]

> +static void mlx5st_pages_slot_complete(struct mlx5st_device *dev)
> +{
[ ... ]
> +		out = calloc(1, outlen);
> +		iovas = calloc(dev->pages_reclaim_npages, sizeof(u64));
> +		VFIO_ASSERT_NOT_NULL(out);
> +		VFIO_ASSERT_NOT_NULL(iovas);
[ ... ]
> +		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 write past the end of the iovas array if the firmware returns a
larger value than expected?

The array is allocated based on dev->pages_reclaim_npages, but the loop bound
uses num_claimed read directly from the untrusted firmware response without
validating it against the allocated size.

[ ... ]

> +static void mlx5st_fw_pages_reclaim(struct mlx5st_device *dev, u16 func_id)
> +{
[ ... ]
> +		in = calloc(1, inlen);
> +		out = calloc(1, outlen);
> +		iovas = calloc(batch, sizeof(u64));
> +		VFIO_ASSERT_NOT_NULL(in);
> +		VFIO_ASSERT_NOT_NULL(out);
> +		VFIO_ASSERT_NOT_NULL(iovas);
[ ... ]
> +		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]
Similar to the issue above in mlx5st_pages_slot_complete(), could this write
out of bounds?

The loop bound num_claimed relies on untrusted firmware output, while the iovas
array is strictly bounded by batch.

[ ... ]

> +static void mlx5st_hca_teardown(struct mlx5st_device *dev)
> +{
> +	u32 out[MLX5_ST_SZ_DW(teardown_hca_out)] = {};
> +	u32 in[MLX5_ST_SZ_DW(teardown_hca_in)] = {};
> +
> +	/* Drain async pages slot, then stop EQ processing */
> +	while (dev->pages_slot_in_use) {
> +		if (!MLX5_GET_ONCE(cmd_queue_entry, dev->pages_cmd_lay,
> +				   ownership))
> +			mlx5st_pages_slot_complete(dev);
> +		else
> +			sched_yield();
> +	}

[Severity: Medium]
Could this cause an indefinite hang if the hardware or firmware stops
responding?

Unlike the regular command polling loop in mlx5st_cmd_exec(), this loop blocks
unconditionally without checking elapsed time or timing out.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0-v6-8af36cf7b73f+e2-mlx5st_jgg@nvidia.com?part=8