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