* [PATCH v2] drm/vkms: Fix UAF between connector configfs rmdir and .detect
2026-07-09 15:06 [PATCH] drm/vkms: Fix UAF between connector configfs rmdir and .detect Ibrahim Hashimov
@ 2026-07-09 19:51 ` Ibrahim Hashimov
0 siblings, 0 replies; 2+ messages in thread
From: Ibrahim Hashimov @ 2026-07-09 19:51 UTC (permalink / raw)
To: louis.chauvet, hamohammed.sa, melissa.srw, simona
Cc: maarten.lankhorst, mripard, tzimmermann, airlied, luca.ceresoli,
harry.wentland, jose.exposito89, dri-devel, linux-kernel, stable
vkms_connector_detect() walks config->connectors via
vkms_config_for_each_connector(), which expands to a plain
list_for_each_entry() over vkms_config::connectors with no lock and no
RCU protection.
Concurrently, rmdir'ing a connector's configfs directory calls
connector_release(), which takes the configfs device mutex
(connector->dev->lock) and calls vkms_config_destroy_connector(),
which does:
list_del(&connector_cfg->link);
kfree(connector_cfg);
Unlike make_crtc_group()/make_plane_group()/make_encoder_group()/
make_connector_group() and the possible_crtcs/possible_encoders
allow_link() callbacks, all of which refuse the operation with -EBUSY
while the device is enabled, connector_release() has no such guard, and
in fact cannot be given one: release() runs after configfs has already
committed to removing the directory, so it cannot fail the rmdir(2)
syscall with -EBUSY. A connector's configfs directory can therefore be
removed, and its vkms_config_connector freed, at any time while the
device is live and its DRM connector is still being probed.
vkms_connector_detect() is reached from userspace by writing "detect"
to /sys/class/drm/<card>-<conn>/status (drm_sysfs status_store() ->
drm_helper_probe_single_connector_modes() -> .detect), which runs under
drm_device.mode_config.mutex. Nothing prevents this from running at the
same time as the configfs rmdir above, since the two paths take
entirely different locks (configfs connector->dev->lock vs. DRM's
mode_config.mutex) over the same vkms_config_connector object. Both
sides require local root/CAP_SYS_ADMIN: the vkms configfs tree is
root-owned (0755) and the connector's sysfs status attribute is
root-writable by default, so this is a local-root race, not an
unprivileged-user or remote issue.
The result is a classic race-into-use-after-free: the unlocked iterator
dereferences connector_cfg->connector or follows connector_cfg->link
into memory that connector_release() has already kfree()'d, or reads
connector_cfg->status out of freed memory. This reproduces as a KASAN
slab-use-after-free in vkms_connector_detect() under a detect-hammer
vs. connector-rmdir stress loop.
A tempting minimal fix is to have vkms_connector_detect() take the same
configfs device lock (connector->dev->lock) that connector_release()
already holds while unlinking/freeing. That would deadlock:
vkms_destroy() (reached from device_enabled_store() while
connector->dev->lock is held) calls drm_dev_unregister() and
drm_atomic_helper_shutdown(), both of which take
drm_device.mode_config.mutex internally. That establishes lock order
configfs-lock -> mode_config.mutex on the teardown path, while
vkms_connector_detect() always runs already holding mode_config.mutex,
so having it additionally acquire the configfs lock would order it
mode_config.mutex -> configfs-lock, the exact reverse (ABBA).
Fix the race with RCU instead, decoupling the free side from the read
side without introducing a new lock-ordering constraint:
- vkms_config_create_connector() links with list_add_tail_rcu()
instead of list_add_tail().
- vkms_config_destroy_connector() unlinks with list_del_rcu() before
it destroys the object, then defers the free with kfree_rcu()
instead of kfree(). It unlinks first so the object is removed from
the list before it is reclaimed; xa_destroy() of
connector_cfg->possible_encoders then runs synchronously between the
unlink and the deferred free, which is safe because no RCU reader
(i.e. vkms_connector_detect()) ever dereferences that field.
- A new vkms_config_for_each_connector_rcu() iterator
(list_for_each_entry_rcu()) is added alongside the existing
vkms_config_for_each_connector() for the other (non-concurrent,
configfs-lock-serialized) call sites, and used only in
vkms_connector_detect(), wrapped in rcu_read_lock()/
rcu_read_unlock().
This is the same pattern DRM core already uses to let connector
iteration (drm_connector_list_iter) survive concurrent connector
teardown without holding mode_config.mutex across the whole walk; here
it is applied locally to vkms's own configfs connector list, matching
the existing "protect the config lists, use RCU when a reader can't
take the writer's lock" discipline already present in the file.
This patch is scoped to the .detect sink. vkms_config_show(), the read
handler for the "vkms_config" debugfs file, also walks the plane, CRTC,
encoder and connector config lists without any lock or RCU protection,
and is subject to a separate, pre-existing use-after-free against a
concurrent configfs rmdir. Fixing it properly requires RCU-converting
the plane, CRTC and encoder teardown paths too:
vkms_config_destroy_plane(), vkms_config_destroy_crtc() and
vkms_config_destroy_encoder() still free their objects non-RCU (plain
list_del() + kfree()), so the RCU conversion done here for connectors
alone is not enough to make that debugfs walk safe. That is left as a
separate follow-up and is not addressed here.
Verified on a v6.19 KASAN-instrumented build: a detect-hammer vs.
connector-rmdir stress loop reliably tripped a "KASAN: slab-use-after-
free in vkms_connector_detect" report before this patch; with the fix
applied, the same stress loop no longer produces any KASAN report.
Fixes: 466f43885ac0 ("drm/vkms: Allow to update the connector status")
Cc: stable@vger.kernel.org
Signed-off-by: Ibrahim Hashimov <security@auditcode.ai>
Assisted-by: AuditCode-AI:2026.07
---
v2: address sashiko-bot review of v1
(https://lore.kernel.org/dri-devel/20260709150637.45052-1-security@auditcode.ai/):
reorder vkms_config_destroy_connector() to unlink (list_del_rcu) before
destroying components (xa_destroy) for remove-before-reclaim ordering; drop the
v1 claim that vkms_config_show() is safe -- it is a separate pre-existing
lockless-traversal issue, noted as out of scope. No change to the RCU .detect fix.
drivers/gpu/drm/vkms/vkms_config.c | 28 ++++++++++++++++++++++++---
drivers/gpu/drm/vkms/vkms_config.h | 22 +++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_connector.c | 16 ++++++++++++++-
3 files changed, 62 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 5a654d6dead8..a44e0567f8a3 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
#include <linux/slab.h>
#include <drm/drm_print.h>
@@ -599,7 +601,12 @@ struct vkms_config_connector *vkms_config_create_connector(struct vkms_config *c
connector_cfg->status = connector_status_connected;
xa_init_flags(&connector_cfg->possible_encoders, XA_FLAGS_ALLOC);
- list_add_tail(&connector_cfg->link, &config->connectors);
+ /*
+ * Paired with vkms_config_for_each_connector_rcu() in
+ * vkms_connector_detect(), which may walk this list without holding
+ * the configfs device lock.
+ */
+ list_add_tail_rcu(&connector_cfg->link, &config->connectors);
return connector_cfg;
}
@@ -607,9 +614,24 @@ EXPORT_SYMBOL_IF_KUNIT(vkms_config_create_connector);
void vkms_config_destroy_connector(struct vkms_config_connector *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 from the list before destroying the object's components, so
+ * the object is removed before it is reclaimed. After list_del_rcu()
+ * no new reader can reach it, and the only RCU reader
+ * (vkms_connector_detect()) never dereferences possible_encoders, so
+ * tearing that down next is safe. Defer the free to after a grace
+ * period so concurrent readers never touch freed memory.
+ */
+ list_del_rcu(&connector_cfg->link);
xa_destroy(&connector_cfg->possible_encoders);
- list_del(&connector_cfg->link);
- kfree(connector_cfg);
+ kfree_rcu(connector_cfg, rcu);
}
EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 8f7f286a4bdd..127076a9280b 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -4,6 +4,7 @@
#define _VKMS_CONFIG_H_
#include <linux/list.h>
+#include <linux/rculist.h>
#include <linux/types.h>
#include <linux/xarray.h>
@@ -108,6 +109,9 @@ struct vkms_config_encoder {
* It can be used to store a temporary reference to a VKMS connector
* during device creation. This pointer is not managed by the
* configuration and must be managed by other means.
+ * @rcu: Used to free the connector configuration after a grace period, so that
+ * vkms_config_for_each_connector_rcu() readers (e.g. .detect) can safely
+ * run concurrently with configfs teardown (see vkms_config_destroy_connector()).
*/
struct vkms_config_connector {
struct list_head link;
@@ -118,6 +122,8 @@ struct vkms_config_connector {
/* Internal usage */
struct vkms_connector *connector;
+
+ struct rcu_head rcu;
};
/**
@@ -152,6 +158,22 @@ struct vkms_config_connector {
#define vkms_config_for_each_connector(config, connector_cfg) \
list_for_each_entry((connector_cfg), &(config)->connectors, link)
+/**
+ * vkms_config_for_each_connector_rcu - RCU-safe iteration over the vkms_config
+ * connectors
+ * @config: &struct vkms_config pointer
+ * @connector_cfg: &struct vkms_config_connector pointer used as cursor
+ *
+ * Unlike vkms_config_for_each_connector(), this may be used without holding
+ * the configfs device lock, from contexts (such as &drm_connector_funcs.detect)
+ * that must not block on it. Callers must wrap the iteration in
+ * rcu_read_lock() / rcu_read_unlock(); see vkms_config_destroy_connector(),
+ * which pairs with this by unlinking with list_del_rcu() and freeing with
+ * kfree_rcu().
+ */
+#define vkms_config_for_each_connector_rcu(config, connector_cfg) \
+ list_for_each_entry_rcu((connector_cfg), &(config)->connectors, link)
+
/**
* vkms_config_plane_for_each_possible_crtc - Iterate over the vkms_config_plane
* possible CRTCs
diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
index b0a6b212d3f4..c43948de2b01 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.c
+++ b/drivers/gpu/drm/vkms/vkms_connector.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
+#include <linux/rcupdate.h>
+
#include <drm/drm_atomic_helper.h>
#include <drm/drm_edid.h>
#include <drm/drm_managed.h>
@@ -26,10 +28,22 @@ static enum drm_connector_status vkms_connector_detect(struct drm_connector *con
*/
status = connector->status;
- vkms_config_for_each_connector(vkmsdev->config, connector_cfg) {
+ /*
+ * configfs can free connector_cfg concurrently (connector_release() ->
+ * vkms_config_destroy_connector(), on an rmdir of the connector's
+ * configfs directory) without taking drm_device.mode_config.mutex,
+ * which this .detect callback is always called under. Walk the RCU-
+ * protected list instead of taking the configfs device lock here: the
+ * two locks are already nested in the opposite order by
+ * vkms_destroy(), so acquiring the configfs lock from under
+ * mode_config.mutex would deadlock.
+ */
+ rcu_read_lock();
+ vkms_config_for_each_connector_rcu(vkmsdev->config, connector_cfg) {
if (connector_cfg->connector == vkms_connector)
status = vkms_config_connector_get_status(connector_cfg);
}
+ rcu_read_unlock();
return status;
}
--
2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 2+ messages in thread