Re: [PATCH v7 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 2 potential issue(s) to consider:
- [Medium] mlx5st: heap buffer overflow in page reclaim if firmware returns unexpected count
- [Low] mlx5st: undefined behavior from shift by 31 on signed integer
--

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

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

Add an mlx5 ConnectX selftest driver that programs VFs and PFs
through the command interface. Create the driver skeleton with
probe(), the command interface, HCA boot sequence, and all resource
allocation up through ALLOC_PD + CREATE_MKEY.

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
@@ -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: Low]
Does this code trigger undefined behavior from a signed integer overflow in
mlx5st_pages_slot_post() (in tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c)?

The hardware configuration sets log_cmdq_size to 5 during initialization,
allowing pages_slot to reach 31. This would overflow the signed 32-bit 1.
Could this literal be 1U to avoid signed overflow?

> +}
[ ... ]
> +static void mlx5st_pages_slot_complete(struct mlx5st_device *dev)
> +{
> +	struct mlx5st_cmd_queue_entry *cmd = dev->pages_cmd_lay;
> +	void *cout;
[ ... ]
> +	if (dev->pages_slot_is_reclaim) {
> +		unsigned int outlen = MLX5_ST_SZ_BYTES(manage_pages_out) +
> +				      dev->pages_reclaim_npages * 8;
> +		unsigned int num_claimed;
> +		unsigned int i;
> +		void *out;
> +		u64 *iovas;
> +
> +		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 overflow the dynamically allocated iovas array in
mlx5st_pages_slot_complete() (in tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c)?

If the hardware responds to a MANAGE_PAGES reclaim command with
output_num_entries greater than dev->pages_reclaim_npages, this unvalidated
output count dictates the iovas assignment loop, overwriting adjacent heap
memory.

> +
> +		mlx5st_fw_pages_free(dev, num_claimed, iovas);
> +		dev_dbg(dev->device, "PAGE_REQUEST: reclaimed %d pages\n",
> +			 num_claimed);
> +
> +		free(iovas);
> +		free(out);
> +	}
[ ... ]
> +static void mlx5st_fw_pages_reclaim(struct mlx5st_device *dev, u16 func_id)
> +{
> +	unsigned int npages = dev->fw_pages_given;
> +	unsigned int total_claimed = 0;
> +
> +	while (npages > 0) {
> +		unsigned int batch = npages < MAX_FW_PAGES_PER_CMD ?
> +					     npages :
> +					     MAX_FW_PAGES_PER_CMD;
> +		unsigned int outlen =
> +			MLX5_ST_SZ_BYTES(manage_pages_out) + batch * 8;
> +		unsigned int inlen = MLX5_ST_SZ_BYTES(manage_pages_in);
> +		unsigned int num_claimed;
> +		unsigned int i;
> +		void *in, *out;
> +		u64 *iovas;
> +
> +		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);
[ ... ]
> +		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]
Does this loop have the same bounds checking issue in
mlx5st_fw_pages_reclaim() (in tools/testing/selftests/vfio/lib/drivers/mlx5/mlx5.c)?

If output_num_entries from the hardware exceeds the requested batch size,
this could overflow the iovas buffer.

> +
> +		mlx5st_fw_pages_free(dev, num_claimed, iovas);
> +		total_claimed += num_claimed;
> +		npages -= num_claimed;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0-v7-c6d30e8ce1e4+3dfa6-mlx5st_jgg@nvidia.com?part=8