Re: [PATCH v1 5/6] drm/rockchip: analogix_dp: Add support for RK3568

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rockchip/dp: unchecked NULL pointer dereference on missing GRF property
--

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

drm/rockchip: analogix_dp: Add support for RK3568

The RK3568 eDP controller does not require any GRF configurations,
so make the "rockchip,grf" property optional.  Also enable the
optional avdd-0v9 and avdd-1v8 supplies which power the eDP PHY.

> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -320,8 +320,9 @@ static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
>  	struct device *dev = dp->dev;
>  	struct device_node *np = dev->of_node;
>  	struct clk *clk;
> +	int ret;
>  
> -	dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> +	dp->grf = syscon_regmap_lookup_by_phandle_optional(np, "rockchip,grf");
>  	if (IS_ERR(dp->grf))
>  		return dev_err_probe(dev, PTR_ERR(dp->grf),
>  				     "failed to get rockchip,grf property\n");

[Severity: High]
Since this changes the lookup to syscon_regmap_lookup_by_phandle_optional(),
dp->grf will be NULL when the property is absent, and the IS_ERR() check will
pass.

Can this cause a NULL pointer dereference for legacy SoCs (like RK3288, RK3399,
or RK3588) if the device tree is missing the property?

For those older SoCs, the chip_data specifies valid GRF fields. During display
initialization, the driver calls rockchip_grf_field_write():

drivers/gpu/drm/rockchip/analogix_dp-rockchip.c:rockchip_grf_field_write() {
	if (!field->valid)
		return 0;

	mask = GENMASK(field->msb, field->lsb);
	val <<= field->lsb;

	return rockchip_grf_write(grf, field->reg, mask, val);
}

This forwards the potentially NULL grf pointer to rockchip_grf_write() and
eventually to regmap_write(), which dereferences it:

drivers/base/regmap/regmap.c:regmap_write() {
	...
	if (!IS_ALIGNED(reg, map->reg_stride))
		return -EINVAL;
	...
}

Should probe validate that dp->grf != NULL if the chip type requires it,
rather than allowing probe to succeed and causing a kernel oops later?

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