Re: [PATCH] drm/vkms: Fix UAF between connector configfs rmdir and .detect

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] RCU-001 violation: `xa_destroy` is called on `possible_encoders` before the object is unlinked from the RCU list.

Pre-existing issues:
- [High] The debugfs helper `vkms_config_show` is vulnerable to a Use-After-Free because it traverses configuration lists without locks or RCU protection.
--

--- Patch [1]: [PATCH] drm/vkms: Fix UAF between connector configfs rmdir and .detect ---
commit 7286ff600c4e0777d618b609fd944dc368ecbf69
Author: Ibrahim Hashimov <security@auditcode.ai>

drm/vkms: Fix UAF between connector configfs rmdir and .detect

This commit addresses a race condition between concurrent configfs directory
removal and drm connector detection in the vkms driver. It introduces RCU
protection for the connector configuration list to decouple the free side
from the read side, avoiding a lock-ordering deadlock.

> No other vkms_config_for_each_connector() call site (vkms_output.c,
> vkms_config.c is_valid()/debugfs helpers) races a concurrent free:
> they all run either at device-build time (single-threaded) or already
> under connector->dev->lock, so they are left untouched to keep the fix
> minimal and scoped to the reported sink.

[Severity: High]
This is a pre-existing issue, but is this statement about debugfs helpers
accurate?

Looking at vkms_config_show() in drivers/gpu/drm/vkms/vkms_config.c, it is
invoked by the VFS seq_read and iterates over the device's planes, CRTCs,
encoders, and connectors locklessly. It does not appear to acquire the
configfs device mutex dev->lock.

Since configfs permits users to rmdir configuration directories at any time,
a concurrent rmdir triggers the configfs release callbacks like
plane_release() and connector_release(). For planes, CRTCs, and encoders,
these callbacks synchronously kfree() the objects.

Could a concurrent rmdir executed during a debugfs read cause
vkms_config_show() to traverse into freed memory?

> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 5a654d6dead84..fc402cf487753 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
[ ... ]
> @@ -608,8 +615,19 @@ EXPORT_SYMBOL_IF_KUNIT(vkms_config_create_connector);
>  void vkms_config_destroy_connector(struct vkms_config_connector *connector_cfg)
>  {
>  	xa_destroy(&connector_cfg->possible_encoders);

[Severity: Medium]
Does this code fully unlink the object from the RCU-protected list before
destroying its components?

In vkms_config_destroy_connector(), xa_destroy() is called on
connector_cfg->possible_encoders before list_del_rcu() removes the object
from the list. This leaves the object partially destructed while it is still
globally visible to concurrent RCU readers traversing the config->connectors
list.

Even if the current RCU reader does not access this field, should the object
be unlinked via list_del_rcu() before calling xa_destroy() to ensure safe
remove-before-reclaim ordering?

> -	list_del(&connector_cfg->link);
> -	kfree(connector_cfg);
> +
> +	/*
> +	 * connector_release() (vkms_configfs.c) can free a connector while
> +	 * vkms_connector_detect() is concurrently walking the connectors list
> +	 * under its own RCU read-side critical section (it cannot take the
> +	 * configfs device lock: it already runs under
> +	 * drm_device.mode_config.mutex, and vkms_destroy() takes the two
> +	 * locks in the opposite order, so plain locking here would deadlock).
> +	 * Unlink with the RCU-safe primitive and defer the free to after a
> +	 * grace period so concurrent readers never dereference freed memory.
> +	 */
> +	list_del_rcu(&connector_cfg->link);
> +	kfree_rcu(connector_cfg, rcu);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709150637.45052-1-security@auditcode.ai?part=1