Re: [PATCH v2 8/8] drm/sun4i: switch DE33 to new bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized blender mode registers for DE33 due to zero plane_cnt.
- [High] Missing device link to the planes device creates a Use-After-Free vulnerability.
--

commit c691a21629b6216c482f2beb09752658d71b6425
Author: Jernej Skrabec <jernej.skrabec@gmail.com>

drm/sun4i: switch DE33 to new bindings

Now that everything is in place, switch DE33 to new bindings.

> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 4071ab38b4ae5..957db09384552 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
[ ... ]
> @@ -638,6 +624,34 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  
>  	clk_prepare_enable(mixer->mod_clk);
>  
> +	if (mixer->cfg->de_type == SUN8I_MIXER_DE33) {
> +		struct platform_device *pdev;
> +		struct device_node *np;
> +		void *data;
> +
> +		np = of_parse_phandle(dev->of_node, "allwinner,planes", 0);
> +		if (!np) {
> +			ret = -ENODEV;
> +			goto err_disable_mod_clk;
> +		}
> +
> +		pdev = of_find_device_by_node(np);
> +		of_node_put(np);
> +		if (!pdev) {
> +			ret = -EPROBE_DEFER;
> +			goto err_disable_mod_clk;
> +		}
> +
> +		data = platform_get_drvdata(pdev);
> +		if (!data) {
> +			put_device(&pdev->dev);
> +			ret = -EPROBE_DEFER;
> +			goto err_disable_mod_clk;
> +		}
> +
> +		mixer->planes_dev = &pdev->dev;

Does this code need a device link (e.g., using device_link_add()) to the
planes device?

Without a device link, unbinding the planes driver via sysfs could free
its devres-managed memory.

Later, sun50i_planes_setup() extracts the planes driver's regmap
(planes->regs) and stores it directly in the DRM layer structures.

If the planes driver is unbound, wouldn't subsequent atomic commits
dereference the dangling layer->regs pointer and cause a use-after-free
regression?

> +	}
> +
>  	list_add_tail(&mixer->engine.list, &drv->engine_list);
>  
>  	/* Reset registers and disable unused sub-engines */
[ ... ]
> @@ -867,16 +886,8 @@ static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = {
>  };
>  
>  static const struct sun8i_mixer_cfg sun50i_h616_mixer0_cfg = {
> -	.lay_cfg = {
> -		.de_type	= SUN8I_MIXER_DE33,
> -		.scaler_mask	= 0xf,
> -		.scanline_yuv	= 4096,
> -	},
>  	.de_type	= SUN8I_MIXER_DE33,
>  	.mod_rate	= 600000000,
> -	.ui_num		= 3,
> -	.vi_num		= 1,

With ui_num and vi_num removed, they will default to 0.

In sun8i_mixer_init(), we compute the total planes:

    plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;

And then initialize the blending modes:

    for (i = 0; i < plane_cnt; i++)
        regmap_write(mixer->engine.regs,
                     SUN8I_MIXER_BLEND_MODE(base, i),
                     SUN8I_MIXER_BLEND_MODE_DEF);

Because plane_cnt will be 0, wouldn't this skip the blender initialization
loop entirely for DE33 hardware?

Could this regression break display composition and alpha blending by leaving
the blender pipes without their default modes?

> -	.map		= {0, 6, 7, 8},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509190015.79086-1-jernej.skrabec@siol.net?part=8