Re: [PATCH v5 33/38] drm/vkms: Introduce configfs for connector EDID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Concurrent update of the EDID blob via ConfigFS and reading via DRM ioctls leads to a Use-After-Free.
- [Medium] The dynamically allocated `connector_cfg->edid` buffer is never freed when a connector configuration is destroyed, leading to a memory leak.
- [Medium] Memory allocation failure when storing an EDID is silently ignored, returning success to userspace and falsely triggering a hotplug event.
- [Medium] Toggling `edid_enabled` at runtime via ConfigFS is broken in both directions and fails to correctly synchronize with the KMS EDID property.
- [Medium] Using a standard ConfigFS string attribute for binary EDID data causes valid 4096-byte EDIDs to be truncated.
- [Medium] Calling `memcpy` with a potentially `NULL` source pointer in `connector_edid_show` triggers undefined behavior.
- [Low] Passing `unsigned int *` to a function expecting `int *` causes an incompatible pointer type compiler warning/error.
--

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

drm/vkms: Introduce configfs for connector EDID

This commit introduces new attributes in configfs to configure the EDID
of a vkms connector. It adds edid_enable and edid attributes.

> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index e1f6e7817f972..f79a796c2017f 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
[ ... ]
> @@ -1218,14 +1218,94 @@ static ssize_t connector_supported_colorspaces_store(struct config_item *item,
[ ... ]
> +static ssize_t connector_edid_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_connector *connector;
> +
> +	connector = connector_item_to_vkms_configfs_connector(item);
> +
> +	scoped_guard(mutex, &connector->dev->lock)
> +	{
> +		unsigned int len = 0;
> +		const u8 *edid = vkms_config_connector_get_edid(connector->config, &len);

[Severity: Low]
Does this trigger an incompatible-pointer-types warning during the build?

The vkms_config_connector_get_edid() function expects an int pointer for
the length, but we are passing an unsigned int pointer here.

> +
> +		memcpy(page, edid, min(len, PAGE_SIZE));

[Severity: Medium]
Could this trigger undefined behavior and a UBSAN warning if edid is not set?

If len is 0, vkms_config_connector_get_edid() returns a NULL pointer, causing
us to call memcpy(page, NULL, 0) unconditionally.

> +		return min(len, PAGE_SIZE);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t connector_edid_store(struct config_item *item,
> +				    const char *page, size_t count)
> +{
> +	struct vkms_configfs_connector *connector;
> +
> +	connector = connector_item_to_vkms_configfs_connector(item);
> +
> +	scoped_guard(mutex, &connector->dev->lock)
> +	{
> +		vkms_config_connector_set_edid(connector->config, page, count);

[Severity: Medium]
Does this code assume the set operation always succeeds? 

Looking at vkms_config_connector_set_edid() in vkms_config.h, it uses 
krealloc() to allocate memory for the EDID but returns void. If the
allocation fails, the old EDID remains unmodified and no error is returned.

This caller then assumes success, returns the written count to userspace,
and falsely triggers a hotplug event below even if the system was out of
memory.

[Severity: High]
Can writing to this attribute cause a use-after-free during concurrent DRM
ioctl reads?

When vkms_config_connector_set_edid() is called here, it holds the
connector->dev->lock. Inside that function, krealloc() frees the old
edid pointer.

However, the DRM read path in vkms_connector_read_block() accesses the EDID
locklessly. If thread A writes a new EDID to configfs, thread B executing
a DRM_IOCTL_MODE_GETCONNECTOR ioctl might dereference the freed pointer while
copying.

> +
> +		if (connector->dev->enabled &&
> +		    vkms_config_connector_get_status(connector->config) !=
> +		    connector_status_disconnected)
> +			vkms_trigger_connector_hotplug(connector->dev->config->dev);
> +	}
> +
> +	return count;
> +}
> +
>  CONFIGFS_ATTR(connector_, status);
>  CONFIGFS_ATTR(connector_, type);
>  CONFIGFS_ATTR(connector_, supported_colorspaces);
> +CONFIGFS_ATTR(connector_, edid_enabled);
> +CONFIGFS_ATTR(connector_, edid);

[Severity: Medium]
Will this string attribute truncate valid 4096-byte EDID payloads?

Since this is defined as a standard CONFIGFS_ATTR rather than a binary
attribute (CONFIGFS_BIN_ATTR), configfs reserves one byte for the null
terminator. A user writing exactly 4096 bytes (a valid 32-block EDID)
would have their write truncated to 4095 bytes.

> diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
> index fe42ab2ba7b10..f726c79955240 100644
> --- a/drivers/gpu/drm/vkms/vkms_connector.c
> +++ b/drivers/gpu/drm/vkms/vkms_connector.c
> @@ -175,6 +175,9 @@ struct vkms_connector *vkms_connector_init_static(struct vkms_device *vkmsdev,
>  
>  	drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs);
>  
> +	if (vkms_config_connector_get_edid_enabled(connector_cfg))
> +		drm_connector_attach_edid_property(&connector->base);
> +

[Severity: Medium]
Is toggling the edid_enabled attribute at runtime broken?

This code only conditionally attaches the property at initialization. If
edid_enabled is false at init, the property is never attached, so changing
it to true later via configfs will fail to expose the EDID.

Conversely, if it is toggled to false at runtime, vkms_conn_get_modes()
fails to call drm_edid_connector_update(connector, NULL). This results in
the DRM core continuing to serve the stale EDID data to userspace.

One additional question about the EDID memory lifecycle:

[Severity: Medium]
Does this change introduce a memory leak when a connector is destroyed?

The dynamically allocated connector_cfg->edid buffer does not appear to be
freed when a connector configuration directory is removed. When configfs
tears down the item, vkms_config_destroy_connector() in vkms_config.c
frees the connector_cfg struct, but it doesn't free the edid field.

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