Re: [PATCH v2] drm/meson: clean up KMS polling on register failure

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

Pre-existing issues:
- [High] NULL pointer dereference in error path if devm_kzalloc() fails to allocate priv
- [High] Use-After-Free due to DRM objects allocated with devm_kzalloc instead of drmm_kzalloc
- [High] Kernel panic/Resource leak on driver unload if initialized without component framework
- [High] Use-After-Free of devm-allocated bridges due to leakage in the global bridge_list
- [High] Use-After-Free due to drm_dev_put() dropping the DRM device before component_unbind_all() accesses it
--

--- Patch [1]: [PATCH v2] drm/meson: clean up KMS polling on register failure ---
commit 86a9a25dd06d99107bc269bc3d6752c0df753219
Author: Myeonghun Pak <[email protected]>

drm/meson: clean up KMS polling on register failure

This commit ensures that drm_kms_helper_poll_fini() is called when
drm_dev_register() fails in meson_drv_bind_master(). This stops the
KMS polling helper before the probe error path unwinds the IRQ and
frees the DRM device.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
> index 49ff9f1f16d32..bcc36f2cdc9a9 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -351,8 +351,10 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>  	platform_set_drvdata(pdev, priv);
>  
>  	ret = drm_dev_register(drm, 0);
> -	if (ret)
> +	if (ret) {
> +		drm_kms_helper_poll_fini(drm);
>  		goto uninstall_irq;
> +	}
>  
>  	drm_client_setup(drm, NULL);
>  

[Severity: High]
This isn't a bug introduced by this patch, but can this error path in
meson_drv_bind_master() trigger a NULL pointer dereference?

If devm_kzalloc() fails to allocate memory for priv, it jumps to free_drm:

drivers/gpu/drm/meson/meson_drv.c:meson_drv_bind_master() {
	...
	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
	if (!priv) {
		ret = -ENOMEM;
		goto free_drm;
	}
	...
}

At the free_drm label, meson_encoder_dsi_remove(priv) is called, which
immediately dereferences the NULL priv pointer:

drivers/gpu/drm/meson/meson_encoder_dsi.c:meson_encoder_dsi_remove() {
	...
	if (priv->encoders[MESON_ENC_DSI]) {
	...
}

[Severity: High]
This is a pre-existing issue, but does using devm_kzalloc() for the DRM plane
risk a use-after-free?

drivers/gpu/drm/meson/meson_plane.c:meson_plane_create() {
	...
	meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
				   GFP_KERNEL);
	...
}

Because devm_kzalloc() ties the memory lifetime to the platform device, if
the device is unbound (e.g., via sysfs) while userspace still holds a DRM
file descriptor open, devres frees the plane structure. Later, when the file
descriptor is closed, the DRM core cleans up and accesses this already-freed
structure to invoke its .destroy hook.

[Severity: High]
This isn't a bug introduced by this patch, but does the driver unload sequence
leak resources if initialized without the component framework?

drivers/gpu/drm/meson/meson_drv.c:meson_drv_probe() {
	...
	if (count && !match)
		return meson_drv_bind_master(&pdev->dev, false);
	...
}

If no matching components are found, it bypasses the component framework.
However, during driver teardown, it only cleans up via the component
framework:

drivers/gpu/drm/meson/meson_drv.c:meson_drv_remove() {
	...
	component_master_del(&pdev->dev, &meson_drv_master_ops);
}

Because it was never added as a component master, meson_drv_unbind() is never
called, leaving the DRM device registered and memory allocated while the module
is unloaded.

[Severity: High]
This is a pre-existing issue, but if drm_simple_encoder_init() fails, does this
leak the bridge in the global bridge_list?

drivers/gpu/drm/meson/meson_encoder_dsi.c:meson_encoder_dsi_probe() {
	...
	drm_bridge_add(&meson_encoder_dsi->bridge);

	meson_encoder_dsi->priv = priv;

	/* Encoder */
	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_dsi->encoder,
				      DRM_MODE_ENCODER_DSI);
	if (ret)
		return dev_err_probe(priv->dev, ret,
				     "Failed to init DSI encoder\n");
	...
}

If drm_simple_encoder_init() fails, the function returns an error without
calling drm_bridge_remove(). Because the bridge is a devm allocation, its
memory is freed on probe failure, but a dangling pointer remains in the
global bridge_list.

[Severity: High]
This isn't a bug introduced by this patch, but can the teardown order cause a
use-after-free when cleaning up components?

drivers/gpu/drm/meson/meson_drv.c:meson_drv_bind_master() {
	...
free_drm:
	drm_dev_put(drm);

	meson_encoder_dsi_remove(priv);
	meson_encoder_hdmi_remove(priv);
	meson_encoder_cvbs_remove(priv);

	if (has_components)
		component_unbind_all(dev, drm);
	...
}

The call to drm_dev_put(drm) drops the final reference and frees the drm
device. Then component_unbind_all(dev, drm) passes this newly freed drm pointer
to the unbind callbacks, which may attempt to access it. This same sequence
also exists in meson_drv_unbind().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic