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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The teardown loop draining the async pages command slot lacks a timeout, risking an infinite hang if the firmware fails to respond.
- [Medium] Missing bounds validation on firmware-returned `output_num_entries` can lead to heap out-of-bounds reads and writes.
- [Low] Signed integer overflow when calculating the doorbell vector for the async pages slot.
--

--- Patch [8]: [PATCH v4 08/10] vfio: selftests: Add mlx5 driver - HW init and command interface ---
commit 7a09d2b7794945d7a16a9b914796b529c3098e0b
Author: Jason Gunthorpe <[email protected]>
Subject: vfio: selftests: Add mlx5 driver - HW init and command interface

This patch introduces a bare-metal mlx5 ConnectX selftest driver for
VFIO, implementing hardware initialization, teardown, and the command
interface.

> 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

[ ... ]

> @@ -710,9 +710,9 @@ 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 risk signed integer overflow? 

If `dev->pages_slot` is 31, shifting the signed literal 1 by 31 bits will
shift into the sign bit, causing undefined behavior in C. Would `1U <<
dev->pages_slot` be safer here?

[ ... ]

> @@ -826,12 +826,12 @@ static void mlx5st_pages_slot_complete(struct mlx5st_device *dev)
>  			mlx5st_cmd_copy_from_mbox(
>  				(u8 *)out + MLX5_CMD_INLINE_SZ,
>  				dev->pages_out_mbox,
>  				outlen - MLX5_CMD_INLINE_SZ);
>  
>  		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 result in out-of-bounds heap accesses if the firmware returns an
unexpectedly large output_num_entries?

Should num_claimed be validated against dev->pages_reclaim_npages before
iterating over iovas and out?

[ ... ]

> @@ -1187,14 +1187,14 @@ static void mlx5st_fw_pages_reclaim(struct mlx5st_device *dev, u16 func_id)
>  		MLX5_SET(manage_pages_in, in, input_num_entries, batch);
>  
>  		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]
Similar to the async completion path above, is it possible for a malfunctioning
device to return a num_claimed larger than the allocated batch size? 

Would it make sense to add a bounds check here to prevent out-of-bounds
array accesses?

[ ... ]

> @@ -1274,13 +1274,13 @@ 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]
Can this loop hang indefinitely? 

Unlike regular commands which use a timeout mechanism, if the firmware fails
to process the async page command and never clears the ownership bit, it looks
like this will yield forever. Should this include a timeout fallback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8