[PATCH] drm/vkms: clear config->dev when vkms_create() fails

[PATCH] drm/vkms: clear config->dev when vkms_create() fails

[Yu Zhang]

vkms_create() stores the new device in config->dev right after
devm_drm_dev_alloc(), but the error path frees the device through
devres_release_group() and faux_device_destroy() without clearing the
pointer. The configfs "enabled" store returns the error without marking
the device as enabled, so the next "enabled" write runs
vkms_config_is_valid() again, which uses config->dev to print its
diagnostics and dereferences the freed drm_device.

Clear config->dev on the error path, as vkms_destroy() does.

Fixes: 13fc9b9745cc ("drm/vkms: Add and remove VKMS instances via configfs")
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 5a640b5..ecc2e47 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -215,6 +215,7 @@ int vkms_create(struct vkms_config *config)
 
 out_devres:
 	devres_release_group(&fdev->dev, NULL);
+	config->dev = NULL;
 out_unregister:
 	faux_device_destroy(fdev);
 	return ret;
-- 
2.43.0

Re: [PATCH] drm/vkms: clear config->dev when vkms_create() fails

[Oarora Etimis]

On 1/1/70 8:00 AM, Oarora Etimis wrote:
> On Sun, Nov 01, 2026 at 01:27:28PM +1000, Yu Zhang wrote:
>> The configfs "enabled" store returns the error without marking the
>> device as enabled, so the next "enabled" write runs
>> vkms_config_is_valid() again, which uses config->dev to print its
>> diagnostics and dereferences the freed drm_device.
> I traced and tested this issue path on drm-misc-next; the dangling pointer is real
> and the one-line fix is correct.
>
> I'd suggest rewording the commit message in two places:
>
>   "dereferences the freed drm_device" is only true for one of the two
>     retry cases.
>
>     a) the retry happens with a config that no longer validates: a
>        validity check takes its failure branch, calls drm_info(), and the
>        freed memory is read.
>
>     b) the retry happens with a still-valid config: vkms_config_is_valid()
>        returns true at its first check, drm_info() is never called, and
>        vkms_create() just stores a new pointer.
>
>     Only a) triggers. The reason: the stale pointer is dereferenced only by
>     drm_info() -- __drm_to_dev() loads drm->dev (drm_print.h:608) and
>     dev_printk() then walks the freed struct device -- and every drm_info()
>     in vkms_config.c sits in a failure branch of a validity check. The
>     ternary itself reads nothing: struct drm_device is the first member of
>     struct vkms_device (vkms_drv.h:252), so
>     config->dev ? &config->dev->drm : NULL is address arithmetic.
>
>> device_enabled_store(), drivers/gpu/drm/vkms/vkms_configfs.c
>>> 	scoped_guard(mutex, &dev->lock) {
>>> 		if (!dev->enabled && enabled) {
>>> 			if (!vkms_config_is_valid(dev->config))
>>> 				return -EINVAL;
>>>
>>> 			ret = vkms_create(dev->config);
>>> 			if (ret)
>>> 				return ret;
>>> 		} else if (dev->enabled && !enabled) {
>>> 			vkms_destroy(dev->config);
>>> 		}
>>>
>>> 		dev->enabled = enabled;
>>> 	}
>     config->dev was introduced by 2df7af93fdad ("drm/vkms: Add vkms_config
>     type"); at that point nothing read the pointer after a failed create.
>     The problem was introduced by 13fc9b9745cc, which changed the lifetime:
>     configfs keeps the config alive across a failed enable, so the pointer
>     stored in vkms_create() outlives the device and is read again by the
>     next write. So Fixes: 13fc9b9745cc is the right tag even though the
>     pointer is older; please keep it, and say so in the message so that it
>     does not read like a mis-attribution.
>
>     Background:
>
>     config->dev is the only backpointer from the configfs side to the
>     drm_device. It is used by the validity diagnostics, by vkms_destroy()
>     and by the connector hotplug path (vkms_configfs.c:555).
>
>     The configfs/drm_device lifetime separation was spelled out when this
>     design was agreed on:
>
>>>> the issue is that configfs and drm_device have independent lifetimes,
>>>> and we cannot have links between the two. The configfs structures might
>>>> have shorter lifetime thatn the vkms_device (and there's no refcounting
>>>> on configfs afaict), which means we cannot have a backpointer from
>>>> vkms_device to configfs.
>>>> The vkms_device->configfs backpointer needs to go, it's fundamentally
>>>> busted.
>     https://lore.kernel.org/all/ZjCh9DGnh2JQ3vtk@phenom.ffwll.local/
>
>     Note that dev->enabled in struct vkms_configfs_device and
>     config->dev != NULL encode the same state, which is why they can
>     diverge here in the first place. Dropping the field and passing the
>     drm_device explicitly to vkms_config_is_valid() is the real fix, but
>     it changes the signature and the hotplug path, so it belongs with the
>     vkms_config lifetime rework rather than here. Clearing the pointer on
>     the error path is the right minimal stopgap.
>
> Thanks,
> Oarora Etimis

Re: [PATCH] drm/vkms: clear config->dev when vkms_create() fails

[Oarora Etimis]

Resending, as my mail client wrapped my previous reply in an extra
level of quoting. The content is unchanged.

On Sun, Nov 01, 2026 at 01:27:28PM +1000, Yu Zhang wrote:
> The configfs "enabled" store returns the error without marking the
> device as enabled, so the next "enabled" write runs
> vkms_config_is_valid() again, which uses config->dev to print its
> diagnostics and dereferences the freed drm_device.

I traced and tested this issue path on drm-misc-next; the dangling pointer is real
and the one-line fix is correct.

I'd suggest rewording the commit message in two places:

 "dereferences the freed drm_device" is only true for one of the two
   retry cases.

   a) the retry happens with a config that no longer validates: a
      validity check takes its failure branch, calls drm_info(), and the
      freed memory is read.

   b) the retry happens with a still-valid config: vkms_config_is_valid()
      returns true at its first check, drm_info() is never called, and
      vkms_create() just stores a new pointer.

   Only a) triggers. The reason: the stale pointer is dereferenced only by
   drm_info() -- __drm_to_dev() loads drm->dev (drm_print.h:608) and
   dev_printk() then walks the freed struct device -- and every drm_info()
   in vkms_config.c sits in a failure branch of a validity check. The
   ternary itself reads nothing: struct drm_device is the first member of
   struct vkms_device (vkms_drv.h:252), so
   config->dev ? &config->dev->drm : NULL is address arithmetic.

> device_enabled_store(), drivers/gpu/drm/vkms/vkms_configfs.c
>> 	scoped_guard(mutex, &dev->lock) {
>> 		if (!dev->enabled && enabled) {
>> 			if (!vkms_config_is_valid(dev->config))
>> 				return -EINVAL;
>>
>> 			ret = vkms_create(dev->config);
>> 			if (ret)
>> 				return ret;
>> 		} else if (dev->enabled && !enabled) {
>> 			vkms_destroy(dev->config);
>> 		}
>>
>> 		dev->enabled = enabled;
>> 	}

   config->dev was introduced by 2df7af93fdad ("drm/vkms: Add vkms_config
   type"); at that point nothing read the pointer after a failed create.
   The problem was introduced by 13fc9b9745cc, which changed the lifetime:
   configfs keeps the config alive across a failed enable, so the pointer
   stored in vkms_create() outlives the device and is read again by the
   next write. So Fixes: 13fc9b9745cc is the right tag even though the
   pointer is older; please keep it, and say so in the message so that it
   does not read like a mis-attribution.

   Background:

   config->dev is the only backpointer from the configfs side to the
   drm_device. It is used by the validity diagnostics, by vkms_destroy()
   and by the connector hotplug path (vkms_configfs.c:555).

   The configfs/drm_device lifetime separation was spelled out when this
   design was agreed on:

>>> the issue is that configfs and drm_device have independent lifetimes,
>>> and we cannot have links between the two. The configfs structures might
>>> have shorter lifetime thatn the vkms_device (and there's no refcounting
>>> on configfs afaict), which means we cannot have a backpointer from
>>> vkms_device to configfs.

>>> The vkms_device->configfs backpointer needs to go, it's fundamentally
>>> busted.

   https://lore.kernel.org/all/ZjCh9DGnh2JQ3vtk@phenom.ffwll.local/

   Note that dev->enabled in struct vkms_configfs_device and
   config->dev != NULL encode the same state, which is why they can
   diverge here in the first place. Dropping the field and passing the
   drm_device explicitly to vkms_config_is_valid() is the real fix, but
   it changes the signature and the hotplug path, so it belongs with the
   vkms_config lifetime rework rather than here. Clearing the pointer on
   the error path is the right minimal stopgap.

Thanks,
Oarora Etimis