Re: [PATCH v5 37/38] drm/vkms: Add connector parent configuration in vkms_config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Error paths in `vkms_connector_hot_add()` free the connector via `__free(kfree)` without tearing down the DRM connector, causing a Use-After-Free and IDA leak.
- [Medium] The MST PATH property generation creates a malformed string and silently drops a valid port ID when the connector hierarchy depth exceeds 16.
- [Medium] Dynamic configuration of connector parent relationships results in inconsistent or stale DRM PATH properties.
--

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

drm/vkms: Add connector parent configuration in vkms_config
    
This patch adds support for configuring the parent connector and port_id
in vkms_config to emulate MST connectors. It updates the PATH property
strings for nested MST configurations and manages dynamic connector
lifecycles.

> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -996,8 +996,16 @@ struct vkms_config_connector *vkms_config_create_connector(struct vkms_config *c
>  }
>  EXPORT_SYMBOL_IF_KUNIT(vkms_config_create_connector);
>  
> -void vkms_config_destroy_connector(struct vkms_config_connector *connector_cfg)
> +void vkms_config_destroy_connector(struct vkms_config *config,
> +				   struct vkms_config_connector *connector_cfg)
>  {
> +	struct vkms_config_connector *connector_cfg_tmp;
> +
> +	vkms_config_for_each_connector(config, connector_cfg_tmp) {
> +		if (connector_cfg_tmp->parent == connector_cfg)
> +			connector_cfg_tmp->parent = NULL;
> +	}
> +

[Severity: Medium]
When dynamically destroying a parent connector, this unlinks it from the
child connectors, but doesn't it leave the active children with stale
DRM PATH properties pointing to destroyed DRM object IDs?

Also, if a child is dynamically added before its parent, the child's PATH
property is silently skipped in vkms_connector_build_path_property() due to
the missing root, and it appears it is never retroactively updated.

[ ... ]

> diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
> --- a/drivers/gpu/drm/vkms/vkms_connector.c
> +++ b/drivers/gpu/drm/vkms/vkms_connector.c
> @@ -8,6 +8,76 @@
>  #include "vkms_config.h"
>  #include "vkms_connector.h"
>  
> +/**
> + * vkms_connector_build_path_property() - Build the PATH property string for MST connectors
[ ... ]
> +	if (!root_cfg || !root_cfg->connector)
> +		return;
> +
> +	root_connector = root_cfg->connector;
> +
> +	len = snprintf(path, sizeof(path), "mst:%d", root_connector->base.base.id);
> +
> +	for (i = port_count - 2; i >= 0; i--) {

[Severity: Medium]
If the connector hierarchy exceeds 16 nodes, port_count will hit the
ARRAY_SIZE(port_ids) limit of 16. Does starting the loop at port_count - 2
(which evaluates to 14) accidentally skip the last recorded array element at
port_ids[15]?

Since the root node is not stored in the array when truncation occurs, it
seems this drops a valid intermediate node from the PATH property string.

> +		int added = snprintf(path + len, sizeof(path) - len,
> +				     "-%u", port_ids[i]);
> +		if (added < 0 || len + added >= sizeof(path))
> +			return;
> +		len += added;
> +	}
> +
> +	drm_connector_set_path_property(&connector->base, path);
> +}

[ ... ]

> @@ -240,6 +312,8 @@ struct vkms_connector *vkms_connector_hot_add(struct vkms_device *vkmsdev,
>  	if (ret)
>  		return ERR_PTR(ret);

[Severity: High]
Can this error path lead to a Use-After-Free and IDA leak?

The connector struct is allocated with __free(kfree). Earlier in this
function, drm_connector_dynamic_init() registers the connector base object
into the DRM mode object IDR and allocates IDAs.

If a subsequent step fails and we return ERR_PTR(ret) here, the __free(kfree)
attribute will directly free the connector memory without calling
drm_connector_cleanup(), leaving dangling pointers in the DRM IDR.

>  
> +	vkms_connector_build_path_property(connector, connector_cfg);
> +
>  	ret = drm_connector_dynamic_register(&connector->base);

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