Re: [PATCH v6 02/28] drm/atomic: Add drm_private_obj name

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm_atomic: ignored drm_atomic_private_obj_init() returns cause list corruption on cleanup
- [Medium] drm_bridge: missing kasprintf() NULL check in __devm_drm_bridge_alloc()
--

commit 7310711925ad29e1494d52640c5914ade7203851
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Add drm_private_obj name

drm_private_obj does not carry a human-readable name, which makes
debug messages hard to interpret when multiple private objects are
registered on the same device.

Add a name field to drm_private_obj, passed through
drm_atomic_private_obj_init() and freed in
drm_atomic_private_obj_fini(). Update the existing callers to pass a
name.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9973becb75aee..53703b23be8f0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
[ ... ]
> @@ -1037,6 +1039,12 @@ int drm_atomic_private_obj_init(struct drm_device *dev,
>  
>  	obj->dev = dev;
>  	obj->funcs = funcs;
> +	obj->name = kstrdup(name, GFP_KERNEL);
> +	if (!obj->name) {
> +		ret = -ENOMEM;
> +		goto err_lock_fini;
> +	}
> +
>  	list_add_tail(&obj->head, &dev->mode_config.privobj_list);
>  
>  	state = obj->funcs->atomic_create_state(obj);

[Severity: High]
Since drm_atomic_private_obj_init() can now return -ENOMEM due to allocation
failure, do all existing callers need to check its return value?

Currently, callers ignore the return value. If kstrdup() fails, the function
returns early without initializing obj->head. When the caller later runs its
teardown path and calls drm_atomic_private_obj_fini(), it will call
list_del(&obj->head), resulting in a null pointer dereference.

> @@ -1051,6 +1059,9 @@ int drm_atomic_private_obj_init(struct drm_device *dev,
>  
>  err_list_del:
>  	list_del(&obj->head);
> +	kfree(obj->name);
> +	obj->name = NULL;
> +err_lock_fini:
>  	drm_modeset_lock_fini(&obj->lock);
>  	return ret;
>  }

[Severity: High]
Similarly, if atomic_create_state() fails and takes the err_list_del path,
list_del(&obj->head) poisons the list pointers. 

Because callers ignore the return value, the subsequent teardown call to
drm_atomic_private_obj_fini() will perform a double-deletion on these
poisoned pointers.

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index e7f9e6d589272..d474d60204110 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
[ ... ]
> @@ -421,6 +422,7 @@ void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
>  	INIT_LIST_HEAD(&bridge->list);
>  	bridge->container = container;
>  	bridge->funcs = funcs;
> +	bridge->name = kasprintf(GFP_KERNEL, "bridge-%s", dev_name(dev));
>  	kref_init(&bridge->refcount);
>  
>  	err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);

[Severity: Medium]
Does kasprintf() need a null check here? 

If the allocation fails under memory pressure, bridge->name will be null,
which is later passed to drm_atomic_private_obj_init() where kstrdup(null)
will return null and fail the initialization.

[ ... ]
> @@ -649,7 +651,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  			goto err_reset_bridge;
>  	}
>  
> -	drm_atomic_private_obj_init(bridge->dev, &bridge->base,
> +	drm_atomic_private_obj_init(bridge->dev, &bridge->base, bridge->name,
>  				    &drm_bridge_priv_state_funcs);
>  
>  	return 0;

[Severity: High]
Here is an example in drm_bridge_attach() where the integer return value of
drm_atomic_private_obj_init() is completely ignored, masking the
initialization failure and setting up the teardown crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=2