[PATCH] drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32

[PATCH] drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32

[Rob Herring (Arm)]

The "sil,i2s-data-lanes" binding and in-tree DTS files use normal
uint32 cells. Reading the array as u8 only works because the values are
small, but it does not match the documented DT element size.

Use the u32 array helper so the driver follows the binding.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/gpu/drm/bridge/sii902x.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 12497f5ce4ff..c27e41573b19 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -862,7 +862,7 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
 		.max_i2s_channels = 0,
 		.no_capture_mute = 1,
 	};
-	u8 lanes[4];
+	u32 lanes[4];
 	int num_lanes, i;
 
 	if (!of_property_present(dev->of_node, "#sound-dai-cells")) {
@@ -871,10 +871,10 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
 		return 0;
 	}
 
-	num_lanes = of_property_read_variable_u8_array(dev->of_node,
-						       "sil,i2s-data-lanes",
-						       lanes, 1,
-						       ARRAY_SIZE(lanes));
+	num_lanes = of_property_read_variable_u32_array(dev->of_node,
+							"sil,i2s-data-lanes",
+							lanes, 1,
+							ARRAY_SIZE(lanes));
 
 	if (num_lanes == -EINVAL) {
 		dev_dbg(dev,
-- 
2.53.0

Re: [PATCH] drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing bounds check on 'sil,i2s-data-lanes' values leads to out-of-bounds array read.
--

commit 460b61bf52ca50a54beb619610726eb8680ea41a
Author: Rob Herring (Arm) <robh@kernel.org>

drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32

The commit updates the sii902x driver to read the "sil,i2s-data-lanes"
device tree property as a u32 array instead of a u8 array. This change makes
the driver follow the documented DT element size and binding specifications.

> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index ea543be4ae3e2..7c90194449e1a 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -862,7 +862,7 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
>  		.max_i2s_channels = 0,
>  		.no_capture_mute = 1,
>  	};
> -	u8 lanes[4];
> +	u32 lanes[4];

[Severity: High]
This isn't a bug introduced by this patch, but does changing the type of lanes
from u8 to u32 increase the severity of a potential out-of-bounds read?

Later in sii902x_audio_codec_init() around line 894 of
drivers/gpu/drm/bridge/sii902x.c, the values parsed from the device tree are
used directly as indices into the i2s_lane_id array:

	for (i = 0; i < num_lanes; i++)
		sii902x->audio.i2s_fifo_sequence[i] |= audio_fifo_id[i] |
			i2s_lane_id[lanes[i]] | SII902X_TPI_I2S_FIFO_ENABLE;

Because i2s_lane_id has a fixed size of 4 elements and there is no validation
on the parsed values, a malformed device tree could provide a value larger
than 3. When lanes was an array of u8, the read offset was limited, but as a
u32 it could hit unmapped memory and trigger a kernel oops during device probe.

Could we add a bounds check to ensure the values are within the correct range?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612215205.1887206-1-robh@kernel.org?part=1

[PATCH] drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32

[Rob Herring (Arm)]

The "sil,i2s-data-lanes" binding and in-tree DTS files use normal
uint32 cells. Reading the array as u8 only works because the values are
small, but it does not match the documented DT element size.

Use the u32 array helper so the driver follows the binding. Changing to
u32 increases a potential OOB access range due to using unchecked
values, so add a check on the read values.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - Add bounds check for property values as reported by sashiko
---
 drivers/gpu/drm/bridge/sii902x.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 9aac44ed5b67..241b472ab13e 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -862,7 +862,7 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
 		.max_i2s_channels = 0,
 		.no_capture_mute = 1,
 	};
-	u8 lanes[4];
+	u32 lanes[4];
 	int num_lanes, i;
 
 	if (!of_property_present(dev->of_node, "#sound-dai-cells")) {
@@ -871,10 +871,10 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
 		return 0;
 	}
 
-	num_lanes = of_property_read_variable_u8_array(dev->of_node,
-						       "sil,i2s-data-lanes",
-						       lanes, 1,
-						       ARRAY_SIZE(lanes));
+	num_lanes = of_property_read_variable_u32_array(dev->of_node,
+							"sil,i2s-data-lanes",
+							lanes, 1,
+							ARRAY_SIZE(lanes));
 
 	if (num_lanes == -EINVAL) {
 		dev_dbg(dev,
@@ -890,9 +890,12 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
 	}
 	codec_data.max_i2s_channels = 2 * num_lanes;
 
-	for (i = 0; i < num_lanes; i++)
+	for (i = 0; i < num_lanes; i++) {
+		if (WARN_ON(lanes[i] >= ARRAY_SIZE(i2s_lane_id)))
+			break;
 		sii902x->audio.i2s_fifo_sequence[i] |= audio_fifo_id[i] |
 			i2s_lane_id[lanes[i]] |	SII902X_TPI_I2S_FIFO_ENABLE;
+	}
 
 	sii902x->audio.mclk = devm_clk_get_optional(dev, "mclk");
 	if (IS_ERR(sii902x->audio.mclk)) {
-- 
2.53.0

Re: [PATCH] drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Using WARN_ON() to validate device tree input causes unnecessary kernel splats and potential kernel panics.
- [Medium] Breaking out of the initialization loop on invalid lane values leaves the audio data structures in an inconsistent state.
--

--- Patch [1]: [PATCH] drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32 ---
commit a4b4eceb945fa6ac0b81acc248b81fdebf849bd9
Author: Rob Herring (Arm) <robh@kernel.org>

drm/bridge: sii902x: Read "sil,i2s-data-lanes" as u32

This patch updates the sii902x bridge driver to read the "sil,i2s-data-lanes"
property as a u32 array instead of a u8 array to match the device tree
bindings. It also introduces a bounds check on the array values to prevent
out-of-bounds accesses.

> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
[ ... ]
> @@ -890,9 +890,12 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
>  	}
>  	codec_data.max_i2s_channels = 2 * num_lanes;
>  
> -	for (i = 0; i < num_lanes; i++)
> +	for (i = 0; i < num_lanes; i++) {
> +		if (WARN_ON(lanes[i] >= ARRAY_SIZE(i2s_lane_id)))

[Severity: Medium]
Is it safe to use WARN_ON() for validating device tree properties?

If panic_on_warn is enabled, couldn't this crash the system when parsing
malformed or untrusted device tree data?

Would it be better to use dev_err() and gracefully fail initialization?

[Severity: Medium]
If this loop breaks early due to an invalid lane, codec_data.max_i2s_channels
remains set to 2 * num_lanes.

Could this cause an inconsistent state where ALSA exposes support for more
channels than the hardware has actually configured in i2s_fifo_sequence?

Should this path return an error (like -EINVAL) to abort audio
initialization, or update the channel count?

> +			break;
>  		sii902x->audio.i2s_fifo_sequence[i] |= audio_fifo_id[i] |
>  			i2s_lane_id[lanes[i]] |	SII902X_TPI_I2S_FIFO_ENABLE;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818234621.1265415-1-robh@kernel.org?part=1