Re: [PATCH v4 2/3] drm/bridge: analogix_dp: Add validation for samsung,lane-count property

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The bounds check for `samsung,lane-count` incorrectly accepts invalid lane counts (like 3) and can be entirely bypassed with large values due to signed comparison, leaving a heap overflow vulnerability unfixed.
--

commit f84cb246760d7a4c631b55b3c7a41c458926dcf4
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Add validation for samsung,lane-count property

This patch adds a range check for the samsung,lane-count device tree property
to avoid invalid values. The validation is only applied when the property
exists to maintain backward compatibility.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c

[ ... ]

> @@ -1260,8 +1261,16 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
>  		 */
>  		of_property_read_u32(dp_node, "samsung,link-rate",
>  				     &video_info->max_link_rate);
> -		of_property_read_u32(dp_node, "samsung,lane-count",
> -				     &video_info->max_lane_count);
> +		ret = of_property_read_u32(dp_node, "samsung,lane-count",
> +					   &video_info->max_lane_count);
> +		if (!ret) {
> +			if (video_info->max_lane_count == 0 ||
> +			    video_info->max_lane_count > LANE_COUNT4) {

[Severity: High]
This is a pre-existing issue, but does this bounds check effectively
prevent all invalid lane counts?

Since max_lane_count is defined as an enum link_lane_count_type, on ARM
architectures (following AAPCS) enums without negative values that fit in a
signed int are represented as signed int. If of_property_read_u32 writes
a large unsigned value like 0xFFFFFFFF into this enum, it is interpreted
as -1. The signed comparison -1 > 4 evaluates to false, which bypasses
the validation.

When this large value is cast back to u32 later in the driver, it could
allow dp->link_train.lane_count to remain at a malicious value read from
the sink's DPCD, eventually leading to a heap buffer overflow when
initializing the cr_loop array which is sized for only 4 elements.

Additionally, does this check inadvertently allow 3, which is an invalid
DisplayPort lane count?

Would it be safer to read the property into a local u32 variable, validate
it strictly, and only then assign it to video_info->max_lane_count?

> +				dev_err(dp->dev, "samsung,lane-count = %d is out of range\n",
> +					video_info->max_lane_count);
> +				return -EINVAL;
> +			}
> +		}
>  		break;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529040530.741336-1-damon.ding@rock-chips.com?part=2