Re: [PATCH v2] media: i2c: vgxy61: reject out of range MIPI CSI-2 lane numbers

[Benjamin Mugnier]

Hi,

A small nitpick, other than that :

Reviewed-by: Benjamin Mugnier <benjamin.mugnier@xxxxxxxxxxx>

Thank you.

Le 22/09/2026 à 08:45, Guo Zihao a écrit :
> vgxy61_tx_from_ep() builds the log2phy and phy2log maps straight from the
> lane numbers in the device tree endpoint, using them as array indices:
> 
>         log2phy[0] = ep.bus.mipi_csi2.clock_lane;
>         phy2log[log2phy[0]] = 0;
>         for (l = 1; l < l_nb + 1; l++) {
>                 log2phy[l] = ep.bus.mipi_csi2.data_lanes[l - 1];
>                 phy2log[log2phy[l]] = l;
>         }
> 
> Both arrays hold VGXY61_NB_POLARITIES (5) entries, and neither lane
> number is checked against that, so an endpoint with a larger value
> writes past the end of the arrays on the stack.
> 
> The endpoint parsing just above validates the number of lanes, but not
> the lane numbers themselves: l_nb is checked against 1, 2 and 4, while
> clock_lane and data_lanes[] are used as-is. A num_data_lanes of 4 with
> an out of range index in the last entry, for example
> data-lanes = <1 2 3 99>, passes the count check and then writes
> phy2log[99]. v4l2_fwnode_endpoint_alloc_parse() does not constrain them
> either: the only use of clock_lane in v4l2-fwnode.c is a BIT(clock_lane)
> duplicate check, which does not reject a value that is merely large.
> 
> Reject a clock lane or any data lane that is not below
> VGXY61_NB_POLARITIES, before the maps are built.
> 
> No Fixes tag. The arrays and the indexing come from the initial driver
> import, 153e4ad44d60 ("media: i2c: Add driver for ST VGXY61 camera
> sensor"), and have not been touched since.
> 
> Reviewed-by: Liu Chao <liuc63@xxxxxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Guo Zihao <guozh23@xxxxxxxxxxxx>
> ---
> v2: add the ST VGXY61 maintainers to the recipients.
> 
>     Correct the note about the lane index range and about other drivers.
>     The indices are unsigned char in struct v4l2_mbus_config_mipi_csi2,
>     so an endpoint value is truncated to 0-255 rather than passed through
>     as a u32, and the comparison now points at vd55g1 and vd56g3, which
>     check clock_lane before building the same kind of map, rather than at
>     an unrelated change in this file.
> 
>     Add the Assisted-by tag.
> 
> The lane numbers come from the "clock-lanes" and "data-lanes" properties
> of the sensor's endpoint node. They are read as u32 by the fwnode helpers
> and stored in unsigned char fields of struct v4l2_mbus_config_mipi_csi2,
> so an endpoint value of 99 arrives as 99 and anything from 5 up writes
> past the five element arrays.
> 
> The binding allows data-lanes 1-4 and does not mention clock-lanes, so a
> correct endpoint does not reach these paths. The checks keep a malformed
> endpoint from writing past the arrays, in the same place the lane count
> check already sits.
> 
> vd55g1 and vd56g3 build the same kind of map from the same properties and
> check clock_lane before the map is built; here the log2phy[0] != 0 check
> runs after the write to phy2log[clock_lane].
> ---
>  drivers/media/i2c/vgxy61.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/i2c/vgxy61.c b/drivers/media/i2c/vgxy61.c
> index 3fb2166c8..ed1205cc6 100644
> --- a/drivers/media/i2c/vgxy61.c
> +++ b/drivers/media/i2c/vgxy61.c
> @@ -1457,9 +1457,20 @@ static int vgxy61_tx_from_ep(struct vgxy61_dev *sensor,
>  	}
>  
>  	/* Build log2phy, phy2log and polarities from ep info */
> +	if (ep.bus.mipi_csi2.clock_lane >= VGXY61_NB_POLARITIES) {

It may be a good time to rename VGXY61_NB_POLARITIES to something a bit
more explicit, like VGXY61_MAX_LANE_ID. Up to you.

> +		dev_err(&client->dev, "invalid clock lane %u\n",
> +			ep.bus.mipi_csi2.clock_lane);
> +		goto error_ep;
> +	}
>  	log2phy[0] = ep.bus.mipi_csi2.clock_lane;
>  	phy2log[log2phy[0]] = 0;
>  	for (l = 1; l < l_nb + 1; l++) {
> +		if (ep.bus.mipi_csi2.data_lanes[l - 1] >=
> +		    VGXY61_NB_POLARITIES) {
> +			dev_err(&client->dev, "invalid data lane %u\n",
> +				ep.bus.mipi_csi2.data_lanes[l - 1]);
> +			goto error_ep;
> +		}
>  		log2phy[l] = ep.bus.mipi_csi2.data_lanes[l - 1];
>  		phy2log[log2phy[l]] = l;
>  	}

-- 
Regards,
Benjamin