Re: [PATCH v2 6/8] drm/sun4i: Add planes driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free vulnerability when the `sun50i-planes` driver is unbound via sysfs.
- [Medium] Race condition during probe exposing uninitialized hardware state.
- [Low] Multiple definition build failure due to duplicate `module_platform_driver()` in the same module.
--

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

drm/sun4i: Add planes driver

This driver serves just as planes sharing manager, needed for Display Engine
3.3 and newer.

> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index bad7497a0d11e..501e3d8679185 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -16,7 +16,8 @@ sun8i-drm-hdmi-y		+= sun8i_hdmi_phy_clk.o
>  
>  sun8i-mixer-y			+= sun8i_mixer.o sun8i_ui_layer.o \
>  				   sun8i_vi_layer.o sun8i_ui_scaler.o \
> -				   sun8i_vi_scaler.o sun8i_csc.o
> +				   sun8i_vi_scaler.o sun8i_csc.o \
> +				   sun50i_planes.o

[ ... ]

> diff --git a/drivers/gpu/drm/sun4i/sun50i_planes.c b/drivers/gpu/drm/sun4i/sun50i_planes.c
> new file mode 100644
> index 0000000000000..6469de1baf030
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun50i_planes.c
> @@ -0,0 +1,201 @@

[ ... ]

> +static int sun50i_planes_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sun50i_planes *planes;
> +	void __iomem *regs;
> +
> +	planes = devm_kzalloc(dev, sizeof(*planes), GFP_KERNEL);
> +	if (!planes)
> +		return -ENOMEM;
> +
> +	planes->quirks = of_device_get_match_data(&pdev->dev);
> +	if (!planes->quirks)
> +		return dev_err_probe(dev, -EINVAL, "Unable to get quirks\n");
> +
> +	planes->mapping = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							  "allwinner,plane-mapping");
> +	if (IS_ERR(planes->mapping))
> +		return dev_err_probe(dev, PTR_ERR(planes->mapping),
> +				     "Unable to get mapping\n");
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	planes->regs = devm_regmap_init_mmio(dev, regs, &sun50i_planes_regmap_config);

If the sun50i-planes driver is unbound via sysfs, devres will clean up this
regmap. Since the driver doesn't seem to integrate with the component
framework or establish device links with the DRM master, could the active DRM
device dereference the freed regmap pointer during atomic plane updates and
cause a use-after-free?

> +	if (IS_ERR(planes->regs))
> +		return PTR_ERR(planes->regs);
> +
> +	dev_set_drvdata(dev, planes);
> +
> +	sun50i_planes_init_mapping(planes);

Is there a potential race condition here? If the DRM mixer driver checks for
the availability of this driver data to determine if the planes driver has
finished probing, it might proceed to use the DRM planes while the hardware's
channel-to-core mapping is still uninitialized.

Should dev_set_drvdata() be called after sun50i_planes_init_mapping()?

> +
> +	return 0;
> +}
> +
> +static struct platform_driver sun50i_planes_platform_driver = {
> +	.probe		= sun50i_planes_probe,
> +	.driver		= {
> +		.name		= "sun50i-planes",
> +		.of_match_table	= sun50i_planes_of_table,
> +	},
> +};
> +module_platform_driver(sun50i_planes_platform_driver);

Since sun50i_planes.o is added to sun8i-mixer-y in the Makefile, it will be
linked into the same module as sun8i_mixer.o. Will invoking
module_platform_driver() here cause a multiple definition linker error for
init_module and cleanup_module?

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