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

From: "Luca Ceresoli" <luca.ceresoli@bootlin.com>
To: "Luca Ceresoli" <luca.ceresoli@bootlin.com>,
	"Damon Ding" <damon.ding@rock-chips.com>
Cc: heiko@sntech.de, dri-devel@lists.freedesktop.org,
	dianders@chromium.org, Laurent.pinchart@ideasonboard.com,
	andrzej.hajda@intel.com, airlied@gmail.com,
	m.szyprowski@samsung.com, simona@ffwll.ch, robh@kernel.org,
	rfoss@kernel.org, sebastian.reichel@collabora.com,
	jernej.skrabec@gmail.com, linux-rockchip@lists.infradead.org,
	devicetree@vger.kernel.org, conor+dt@kernel.org, jonas@kwiboo.se,
	maarten.lankhorst@linux.intel.com, mripard@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	dmitry.baryshkov@oss.qualcomm.com, neil.armstrong@linaro.org,
	hjc@rock-chips.com, linux-kernel@vger.kernel.org,
	tzimmermann@suse.de, andy.yan@rock-chips.com, krzk+dt@kernel.org,
	nicolas.frattaroli@collabora.com
Subject: Re: [PATCH v4 2/3] drm/bridge: analogix_dp: Add validation for samsung,lane-count property
Date: Sat, 30 May 2026 15:38:05 +0200	[thread overview]
Message-ID: <DIW1WGLL0GW5.1BGU194UXN0HO@bootlin.com> (raw)
In-Reply-To: <178014803941.21632.16225608049285101452.b4-review@b4>

On Sat May 30, 2026 at 3:33 PM CEST, Luca Ceresoli wrote:
> On Fri, 29 May 2026 12:05:29 +0800, Damon Ding <damon.ding@rock-chips.com> wrote:
>
> Hello Damon,
>
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index 8cf6b73bceac..699a7f380c56 100644
>> --- 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) {
>
> This sashiko report seems to me valid.

Meh, messed up with 'b4 review' :-/ Apologies

"This sashiko report" [0] was about an enum being signed, so '== 0' could miss
negative numbers coming from bogus DT values higher than 1^31.

>
> But I'n no DP expert, I have no idea whether this ther one is valid.

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

>> +				dev_err(dp->dev, "samsung,lane-count = %d is out of range\n",
>> +					video_info->max_lane_count);
>> +				return -EINVAL;
>> +			}
>> +		}
>
> As reported by sashiko, 'count == 0' should be 'count <= 0', being an enum.
>
> Additionally I'd avoid the nested if, and I think using dev_err_probe() is
> correct here (we are only called by probe functions), so it all could
> become:
>
>    if (ret || count <= 0 || count > LANE_COUNT0)
>         return dev_err_probe(...);
>
> There are other sashiko reports to patch 3, and at least one seems valid to
> me. Can you either fix them in the next iteration or elaborate on why the
> code is correct there?
>
> Thanks!

[0] https://sashiko.dev/#/patchset/20260529040530.741336-1-damon.ding%40rock-chips.com

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip