Re: [PATCH v5 36/38] drm/vkms: Introduce configfs for dynamic connector creation

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Calling drm_mode_config_reset() dynamically at runtime destroys active atomic states, leading to Use-After-Free.
- [High] Mixing goto-based error handling with scoped_guard causes a data race on rollback.
- [High] Removing a connector via rmdir bypasses validation checks, leaving the device in an invalid state.
- [High] Uncaught NULL pointer dereference due to incorrect IS_ERR() check on memory allocations.
- [High] Using __free(kfree) on a dynamically initialized DRM connector causes memory leaks and list corruption.
- [High] Dangling pointer in connector_cfg->connector causes state corruption via aliasing.
- [Medium] Missing rollback when configfs internal allocation fails after hot-attaching an encoder.
- [Medium] Dropping a symlink for an active dynamic connector removes the encoder from ConfigFS but fails to detach it from the live DRM connector.

Pre-existing issues:
- [High] Userspace can rmdir static connectors while the DRM device is activated, causing use-after-free.
- [High] Lockless iteration over vkmsdev->config->connectors races with configfs item removal.
--

commit fa75c62308f0301b4607b37a03a00f5ce157f6a0
Author: Louis Chauvet <louis.chauvet@bootlin.com>

drm/vkms: Introduce configfs for dynamic connector creation

This commit adds two configfs attributes to connectors to allow emulating
dynamic connector creation after the device. The dynamic attribute marks a
connector as dynamic, and the enabled attribute can set it as present or not
at runtime.

> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index f79a796c2017f..f92b172e75cf3 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
[ ... ]
> @@ -1290,6 +1296,107 @@ static ssize_t connector_edid_store(struct config_item *item,
[ ... ]
> +			if (!was_enabled && enabled) {
> +				// Adding the connector
> +				connector_cfg->connector = vkms_connector_hot_add(connector->dev->config->dev,
> +										  connector_cfg);

[Severity: High]
Is it possible for vkms_connector_hot_add() to return a NULL pointer that gets
missed?

Looking at vkms_connector_hot_add() in drivers/gpu/drm/vkms/vkms_connector.c:

drivers/gpu/drm/vkms/vkms_connector.c:vkms_connector_hot_add() {
	...
	connector = kzalloc_obj(*connector, GFP_KERNEL);
	if (IS_ERR(connector))
		return connector;
	...
}

kzalloc_obj() returns NULL on failure, but the IS_ERR() check evaluates to
false for NULL. The NULL pointer would then be immediately passed to
drm_connector_dynamic_init(), resulting in a kernel panic.

[Severity: High]
Does vkms_connector_hot_add() leak memory or corrupt lists on error?

In the same function:

drivers/gpu/drm/vkms/vkms_connector.c:vkms_connector_hot_add() {
	struct vkms_connector __free(kfree) * connector = NULL;
	...
}

If drm_connector_dynamic_init() succeeds, it registers the connector in core
DRM structures. If a subsequent initialization step fails, the function
returns an error, triggering the __free(kfree) cleanup.

The raw kfree() frees the memory without invoking drm_connector_cleanup() or
dropping references, leaving dangling pointers in the DRM core lists and
leaking internal property allocations.

> +				if (IS_ERR(connector_cfg->connector)) {
> +					count = PTR_ERR(connector_cfg->connector);
> +					goto rollback;
> +				}
> +			} else if (was_enabled && !enabled) {
> +				vkms_connector_hot_remove(connector->dev->config->dev,
> +							  connector_cfg->connector);

[Severity: High]
Does this leave a dangling pointer in connector_cfg->connector?

When a dynamic connector is disabled, vkms_connector_hot_remove() unregisters
and frees the DRM connector. However, connector_cfg->connector is not cleared.

Reusing memory for a new dynamic connector may result in the same pointer
address, and iteration logic that matches against it will match the dangling
pointer, corrupting connector configuration states.

[Severity: Critical]
Is it safe to call drm_mode_config_reset() dynamically at runtime?

Looking at vkms_connector_hot_remove() and vkms_connector_hot_attach_encoder():

drivers/gpu/drm/vkms/vkms_connector.c:vkms_connector_hot_remove() {
	...
	drm_mode_config_reset(&vkmsdev->drm);
	...
}

This helper loops over all CRTCs, planes, and connectors, unconditionally
freeing their current active atomic states and allocating fresh empty states.

Executing this while userspace has an active modeset destroys the structures
currently in use without proper teardown or synchronization, leading to
use-after-free regressions.

> +			}
> +		}
> +	}
> +	return count;
> +
> +rollback:
> +	vkms_config_connector_set_enabled(connector_cfg, was_enabled);

[Severity: High]
Can this modification on rollback race with concurrent accesses?

A goto rollback jumps out of the scoped_guard block, releasing the lock
implicitly:

drivers/gpu/drm/vkms/vkms_configfs.c:connector_enabled_store() {
	scoped_guard(mutex, &connector->dev->lock) {
...
				if (IS_ERR(connector_cfg->connector)) {
					count = PTR_ERR(connector_cfg->connector);
					goto rollback;
				}
...
	}
...
rollback:
	vkms_config_connector_set_enabled(connector_cfg, was_enabled);

The rollback executes without the lock held. This violates the subsystem rule
against mixing goto and scope-based cleanup helpers.

[ ... ]
> @@ -1306,19 +1415,28 @@ static struct configfs_attribute *connector_item_attrs[] = {
[ ... ]
>  static void connector_release(struct config_item *item)
>  {
>  	struct vkms_configfs_connector *connector;
> +	struct vkms_config_connector *connector_cfg;
>  	struct mutex *lock;
>  
>  	connector = connector_item_to_vkms_configfs_connector(item);
> +	connector_cfg = connector->config;
>  	lock = &connector->dev->lock;
>  
>  	scoped_guard(mutex, lock) {
> +		if (connector->dev->enabled && connector_cfg->dynamic && connector_cfg->enabled)
> +			vkms_connector_hot_remove(connector->dev->config->dev,
> +						  connector_cfg->connector);
> +

[Severity: High]
Does this allow userspace to bypass topological validation?

Disabling a connector via the enabled sysfs attribute correctly checks
vkms_config_is_valid(). However, removing a dynamic connector entirely via
rmdir calls connector_release(), which performs vkms_connector_hot_remove()
unconditionally.

This bypasses validation checks (e.g., removing the only connected connector),
breaking the internal DRM configuration rules while the device is running.

>  		vkms_config_destroy_connector(connector->config);
> +

[Severity: High]
This is a pre-existing issue, but can userspace remove static connectors while
the DRM device is activated, causing a use-after-free?

Userspace can remove a static connector via rmdir while the DRM device is
fully enabled. connector_release() bypasses vkms_connector_hot_remove() for
static connectors and immediately destroys the configuration structure.

The active DRM device retains pointers to the destroyed configuration and will
dereference it.

[Severity: High]
This is a pre-existing issue, but does lockless iteration over connectors race
with configfs item removal?

When userspace invokes rmdir on a connector, connector_release() frees the
connector_cfg structure. Concurrently, functions like vkms_connector_detect()
and vkms_conn_get_modes() iterate over vkmsdev->config->connectors locklessly:

drivers/gpu/drm/vkms/vkms_connector.c:vkms_connector_detect() {
	...
	vkms_config_for_each_connector(vkmsdev->config, connector_cfg) {
		if (connector_cfg->connector == vkms_connector)
			status = vkms_config_connector_get_status(connector_cfg);
	}
	...
}

This lockless iteration traverses freed memory, causing a use-after-free
regression.

>  		kfree(connector);
>  	}
>  }
> @@ -1337,6 +1455,7 @@ static int connector_possible_encoders_allow_link(struct config_item *src,
[ ... ]
>  	scoped_guard(mutex, &connector->dev->lock) {
> -		if (connector->dev->enabled)
> -			return -EBUSY;
> +		if (connector->dev->enabled && connector_cfg->enabled) {
> +			if (!connector_cfg->dynamic)
> +				return -EBUSY;
> +
> +			ret = vkms_connector_hot_attach_encoder(connector->dev->config->dev,
> +								connector->config->connector,
> +								encoder->config->encoder);
> +			if (ret)
> +				return ret;
> +		}
>  
>  		ret = vkms_config_connector_attach_encoder(connector->config,
>  							   encoder->config);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Is there a missing rollback if the internal configfs allocation fails?

vkms_connector_hot_attach_encoder() updates the live hardware state.
Immediately after, vkms_config_connector_attach_encoder() performs a memory
allocation to update the configfs state.

If this allocation fails, the error is returned without undoing the hardware
attachment, leaving the active DRM device out of sync with the configfs
representation.

[Severity: Medium]
Does dropping a symlink for an active dynamic connector fail to detach the
encoder from the live DRM connector?

Looking at the corresponding drop function:

drivers/gpu/drm/vkms/vkms_configfs.c:connector_possible_encoders_drop_link() {
	...
	scoped_guard(mutex, &connector->dev->lock) {
		vkms_config_connector_detach_encoder(connector->config,
						     encoder->config);
	}
}

It successfully removes the encoder from the internal configfs state, but if
the device is enabled, it fails to interact with the DRM core to actually
detach the encoder from the hardware state.

This creates a state divergence where configfs thinks the encoder is unlinked,
but DRM continues to use it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627-vkms-all-config-v5-0-854aa0840926@bootlin.com?part=36