Re: [PATCH v2 1/11] drm: logicvc: Use devm_of_reserved_mem_device_init()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-free of layer structures (containing drm_plane) during driver cleanup due to incorrect devm vs. drmm initialization order.
- [High] Concurrent probe overwrites shared static global regmap_config, causing a data race.
--

commit 3a0c473b709e63281231182c727f9e5d25ce8b6f
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

drm: logicvc: Use devm_of_reserved_mem_device_init()

Switch to devm_of_reserved_mem_device_init() so the reserved memory region
is released automatically on probe failure or device unbind. Replace manual
cleanup labels with error_early.

> diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
> index bbebf4fc7f51a..d6dbe52ff0a99 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_drm.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
[ ... ]
> @@ -327,14 +327,14 @@ static int logicvc_drm_probe(struct platform_device *pdev)
[ ... ]
>  		base = devm_ioremap_resource(dev, &res);
>  		if (IS_ERR(base)) {
>  			dev_err(dev, "Failed to map I/O base\n");
>  			ret = PTR_ERR(base);
> -			goto error_reserved_mem;
> +			goto error_early;
>  		}
>  
>  		logicvc_drm_regmap_config.max_register = resource_size(&res) -

[Severity: High]
This isn't a bug introduced by this patch, but does this concurrent probe 
overwrite the shared static global logicvc_drm_regmap_config?

Modifying a static global structure without locking can cause a data race if 
multiple devices probe concurrently (e.g., if multiple IP cores are defined 
in the device tree). 

Depending on the race window, one device instance might be initialized with 
the max_register value of another instance, leading to either restricted MMIO 
access or out-of-bounds MMIO access.

> @@ -345,21 +345,21 @@ static int logicvc_drm_probe(struct platform_device *pdev)
[ ... ]
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		ret = -ENODEV;
> -		goto error_reserved_mem;
> +		goto error_early;
>  	}
>  
>  	logicvc = devm_drm_dev_alloc(dev, &logicvc_drm_driver,
>  				     struct logicvc_drm, drm_dev);

[Severity: High]
This is a pre-existing issue, but can this devm_drm_dev_alloc() call lead to 
a use-after-free of layer structures during driver cleanup?

This call establishes the point in the devres stack where drm_dev_put() will 
execute. Because logicvc_layer_init() allocates layer structures with 
devm_kzalloc() after devm_drm_dev_alloc() executes:

drivers/gpu/drm/logicvc/logicvc_layer.c:logicvc_layer_init() {
    ...
    layer = devm_kzalloc(dev, sizeof(*layer), GFP_KERNEL);
    ...
}

devres will execute in LIFO order on unbind. This means devm_kfree() frees 
the layer memory before the DRM device's devres action calls drm_dev_put().

When the DRM device is finally destroyed, drmm actions execute (including 
drm_mode_config_cleanup()), which iterates over registered planes and calls 
their destroy hook (drm_plane_cleanup). Since the plane is embedded in the 
already-freed layer struct, this causes a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902201640.2024648-1-mukesh.ojha@oss.qualcomm.com?part=1