Re: [PATCH v5 32/38] drm/vkms: Introduce config for connector EDID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The custom EDID buffer allocated in `vkms_config_connector_set_edid()` is never freed when the connector configuration is destroyed, leading to a memory leak.
- [Medium] Disabling EDID on a connector fails to clear the previously set EDID property from the DRM connector object.
- [Low] Pointer type mismatch when passing `&edid_len` to `vkms_config_connector_get_edid()`, which causes an incompatible-pointer-types warning.
- [Low] `vkms_config_connector_set_edid()` fails silently if memory allocation fails, providing no way for callers to handle the error.
--

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

    drm/vkms: Introduce config for connector EDID
    
    Allows configuration of EDID for each connector.

> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index 80bbc13aad79b..7e361b6e45b0e 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
[ ... ]
> @@ -274,6 +280,75 @@ vkms_config_connector_get_supported_colorspaces(const struct vkms_config_connect
[ ... ]
> +static inline void
> +vkms_config_connector_set_edid(struct vkms_config_connector *connector_cfg,
> +			       const u8 *edid, unsigned int len)
> +{
> +	if (len) {
> +		void *edid_tmp = krealloc(connector_cfg->edid, len, GFP_KERNEL);

[Severity: Medium]
Does this code leak the memory allocated for the EDID buffer?

The buffer is dynamically allocated here, but it doesn't appear to be freed
when the connector configuration is torn down in
vkms_config_destroy_connector(). This could cause a leak when userspace
creates and destroys dynamic connectors with EDID data via configfs.

[Severity: Low]
Should this function return an error code rather than void?

If the krealloc() allocation fails, it fails silently. Callers (like
userspace writing to the configfs attribute) will assume the operation
succeeded, leaving the connector with an inconsistent or outdated EDID.

> +
> +		if (edid_tmp) {
> +			connector_cfg->edid = edid_tmp;
> +			memcpy(connector_cfg->edid, edid, len);
> +			connector_cfg->edid_len = len;
> +		}
> +	} else {
> +		kfree(connector_cfg->edid);
> +		connector_cfg->edid = NULL;
> +		connector_cfg->edid_len = len;
> +	}
> +}
> +
[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
> index a1f08167def1d..fe42ab2ba7b10 100644
> --- a/drivers/gpu/drm/vkms/vkms_connector.c
> +++ b/drivers/gpu/drm/vkms/vkms_connector.c
> @@ -42,13 +42,55 @@ static const struct drm_connector_funcs vkms_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> +static int vkms_connector_read_block(void *context, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct vkms_config_connector *config = context;
> +	unsigned int edid_len;
> +	const u8 *edid = vkms_config_connector_get_edid(config, &edid_len);

[Severity: Low]
Does this cause a compiler warning about incompatible pointer types?

The variable edid_len is declared as unsigned int, but its address is passed
to vkms_config_connector_get_edid(), which expects an int pointer for its
length parameter.

> +
> +	if (block * len + len > edid_len)
> +		return 1;
> +	memcpy(buf, &edid[block * len], len);
> +	return 0;
> +}
> +
>  static int vkms_conn_get_modes(struct drm_connector *connector)
>  {
[ ... ]
> +			/*
> +			 * Unconditionally update the connector. If the EDID was read
> +			 * successfully, fill in the connector information derived from the
> +			 * EDID. Otherwise, if the EDID is NULL, clear the connector
> +			 * information.
> +			 */
> +			drm_edid_connector_update(connector, drm_edid);
> +
> +			count = drm_edid_connector_add_modes(connector);
> +
> +			drm_edid_free(drm_edid);
> +		} else {
> +			count = drm_add_modes_noedid(connector, XRES_MAX, YRES_MAX);
> +			drm_set_preferred_mode(connector, XRES_DEF, YRES_DEF);

[Severity: Medium]
Should we call drm_edid_connector_update(connector, NULL) in this else branch?

If a connector is configured with EDID enabled, drm_edid_connector_update()
is called above to attach the property. If it is later dynamically changed
to disable EDID, this branch fails to clear the DRM connector object's EDID
property, which could leave stale EDID data exposed to userspace.

> +		}
> +	}
>  
>  	return count;
>  }

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