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

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

[Guo Zihao]

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@xiaopeng.com>
Assisted-by: LLM
Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
---
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) {
+		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;
 	}
-- 
2.50.1

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@foss.st.com>

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@xiaopeng.com>
> Assisted-by: LLM
> Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
> ---
> 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