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

[Benjamin Mugnier]

Hi,

Please add the driver maintainers in copy while submitting a patch so it
pops up on our mail filters. I added Sylvain here.

Le 18/09/2026 à 08:03, 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.

Correct me if I'm wrong, but they are checked at line 1453, which is in
the same function by the way :

  l_nb = ep.bus.mipi_csi2.num_data_lanes;
  if (l_nb != 1 && l_nb != 2 && l_nb != 4) {
    dev_err(&client->dev, "invalid data lane number %d\n", l_nb);
    goto error_ep;
  }

So there is not need to double check it after.

Also, it looks like your patch is LLM generated, if this is the case
please disclose it as per [1].

[1] https://docs.kernel.org/process/coding-assistants.html

> 
> 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.
> 
> 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, with the same dev_err() and goto the lane count
> check uses.
> 
> 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>
> Signed-off-by: Guo Zihao <guozh23@xxxxxxxxxxxx>
> ---
> The lane numbers come from the "clock-lanes" and "data-lanes" properties
> of the sensor's endpoint node. Both are read as u32 by the fwnode
> helpers, so a value like 5 or 0xffffffff reaches vgxy61_tx_from_ep()
> unchanged.
> 
> For a module built into a device whose DT the machine owner controls this
> is not a trust boundary, so the practical impact is a malformed DT
> corrupting the stack rather than an attacker escalating. It is the same
> class of fix that the driver already applies to the lane count two lines
> above, and that other CSI-2 drivers apply to their lane numbers.
> 
> dab65dfbf9c8 is a recent example of this file taking a defensive check
> for input it cannot control.
> 
>  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) {
> +		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