Re: [PATCH v5 31/38] drm/vkms: Introduce configfs for connector supported colorspace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing validation for `supported_colorspaces` against connector types, leading to silent configuration failure.
- [Low] Passing an `int *` to `kstrtouint()` instead of an `unsigned int *`, and mismatched variable types for colorspaces.
--

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

drm/vkms: Introduce configfs for connector supported colorspace
    
Add the attribute supported_colorspaces to configure the supported
colorspace of a connector. It will allows emulating some HDR features of
displays. This feature is only supported for HDMI, DP and eDP connectors.

> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 8b414f1950e8e..e1f6e7817f972 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
[ ... ]
> @@ -1180,12 +1180,52 @@ static ssize_t connector_type_store(struct config_item *item,
>  	return count;
>  }
>  
> +static ssize_t connector_supported_colorspaces_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_connector *connector;
> +	int colorspaces;
> +
> +	connector = connector_item_to_vkms_configfs_connector(item);
> +
> +	scoped_guard(mutex, &connector->dev->lock)
> +	colorspaces = vkms_config_connector_get_supported_colorspaces(connector->config);

[Severity: Low]
Does this code assign a u32 value to a signed int?

vkms_config_connector_get_supported_colorspaces() returns a u32, but
colorspaces is declared as an int.

> +
> +	return sprintf(page, "%u", colorspaces);
> +}
> +
> +static ssize_t connector_supported_colorspaces_store(struct config_item *item,
> +						     const char *page, size_t count)
> +{
> +	struct vkms_configfs_connector *connector;
> +	int val, ret;
> +
> +	connector = connector_item_to_vkms_configfs_connector(item);
> +
> +	ret = kstrtouint(page, 10, &val);

[Severity: Low]
Does this code pass an int pointer to an API expecting an unsigned int pointer?

kstrtouint() expects an unsigned int * for its third argument, but val is
declared as an int.

> +	if (ret)
> +		return ret;
> +
> +	if ((val & -BIT(DRM_MODE_COLORIMETRY_COUNT)) != 0)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &connector->dev->lock) {
> +		if (connector->dev->enabled)
> +			return -EBUSY;
> +
> +		vkms_config_connector_set_supported_colorspaces(connector->config, val);

[Severity: Medium]
Since this feature is only supported for HDMI, DP, and eDP connectors, is
there any validation that prevents users from configuring this on unsupported
connector types?

If users configure this attribute on an unsupported connector (like the default
VIRTUAL type), it appears it will be accepted here but silently ignored later
when the device is initialized.

Would it make sense to reject unsupported configurations in
vkms_config_is_valid() or similar validation logic to prevent misleading
userspace?

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

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