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 <[email protected]> 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/[email protected]?part=2
