Re: [PATCH v5 23/38] drm/vkms: Introduce configfs for plane format

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Strict length validation in `plane_supported_formats_store` rejects valid shell inputs containing trailing newlines.
- [High] Type punning between a `char` array and `int *` causes unaligned memory accesses and produces corrupted FourCC codes on big-endian architectures.
- [High] Missing `plane->dev->enabled` check in `plane_supported_formats_store` allows userspace to modify formats while the device is active, causing a Use-After-Free.
- [Medium] Incorrect pointer arithmetic in `vkms_configfs_parse_next_format` results in a negative length, defeating bounds checking for `strnchr`.
- [Low] `vkms_configfs_parse_next_format` is completely unused in the operational driver and causes a build failure when KUnit is disabled.
--

commit a7abf4a30e4c9317197078c2d2f5f96c79800929
Author: Louis Chauvet <louis.chauvet@bootlin.com>
Subject: drm/vkms: Introduce configfs for plane format

This commit introduces a configfs interface to configure the available
formats per planes in VKMS. It adds parsing logic and test cases to
allow userspace to test multiple hardware configurations.

> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c

[ ... ]

> +static ssize_t plane_supported_formats_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_plane *plane;
> +
> +	plane = plane_item_to_vkms_configfs_plane(item);
> +
> +	page[0] = '\0';
> +
> +	scoped_guard(mutex, &plane->dev->lock) {
> +		u32 *formats = vkms_config_plane_get_supported_formats(plane->config);
> +
> +		for (int i = 0;
> +		     i < vkms_config_plane_get_supported_formats_count(plane->config);
> +		     i++) {
> +			char tmp[6] = { 0 };
> +			const ssize_t ret = snprintf(tmp, ARRAY_SIZE(tmp), "%.*s\n",
> +					       (int)sizeof(*formats),
> +					       (char *)&formats[i]);

[Severity: High]
Does this cast to a char pointer cause endianness issues on big-endian
architectures?

Since DRM FourCC codes are strictly little-endian, interpreting the integer
directly as a character array might reverse the format string on big-endian
machines.

> +			if (ret < 0)
> +				return ret;

[ ... ]

> +VISIBLE_IF_KUNIT
> +int vkms_configfs_parse_next_format(const char *page, const char *page_end, char **out)

[Severity: Low]
Since this function isn't called anywhere in the operational driver paths, does
it cause a -Wunused-function build failure when CONFIG_KUNIT is disabled and
VISIBLE_IF_KUNIT evaluates to static?

> +{
> +	int count = page - page_end;

[Severity: Medium]
Since page comes before page_end in memory, won't this calculation result in a
negative count?

When passed to strnchr() which takes a size_t, won't this negative value
become a massive positive integer and bypass the bounds check?

> +	char *tmp_plus = strnchr(page, count, '+');
> +	char *tmp_minus = strnchr(page, count, '-');

[ ... ]

> +static ssize_t plane_supported_formats_store(struct config_item *item,
> +					     const char *page, size_t count)
> +{
> +	struct vkms_configfs_plane *plane;
> +
> +	plane = plane_item_to_vkms_configfs_plane(item);
> +	int ret = 0;
> +
> +	if (count < 2 || count > 5)
> +		return -EINVAL;
> +
> +	if (page[0] != '+' && page[0] != '-')
> +		return -EINVAL;
> +
> +	if (page[1] == '*' && count != 2)
> +		return -EINVAL;

[Severity: High]
Does this rigid length validation reject standard shell inputs?

When users configure the format using standard commands like
echo "+AR24" > supported_formats, the shell appends a trailing newline,
making the count 6 instead of 5. For echo "+*", the count becomes 3 instead
of 2.

Will this prevent userspace from using the interface as documented?

> +
> +	scoped_guard(mutex, &plane->dev->lock) {

[Severity: High]
Should there be a check here to ensure the device is not enabled, similar to
other configfs store functions like plane_zpos_enabled_store()?

If userspace modifies or clears the supported formats while the virtual DRM
device is active, vkms_config_plane_remove_all_formats() frees the formats
array. Could this lead to a use-after-free if the rendering pipeline is
concurrently accessing it?

For example, shouldn't we have:
	if (plane->dev->enabled)
		return -EBUSY;

> +		char fmt[4] = {' ', ' ', ' ', ' '};
> +
> +		memcpy(fmt, &page[1], min(count - 1, 4));
> +
> +		if (page[0] == '+' && page[1] == '*')
> +			ret = vkms_config_plane_add_all_formats(plane->config);
> +		else if (page[0] == '-' && page[1] == '*')
> +			vkms_config_plane_remove_all_formats(plane->config);
> +		else if (page[0] == '+')
> +			ret = vkms_config_plane_add_format(plane->config, *(int *)fmt);

[Severity: High]
Does casting this 4-byte character array to an int pointer risk alignment
faults?

If the character array on the stack is not 4-byte aligned, won't this cause
a panic on architectures that enforce strict alignment?

Additionally, similar to the show() function, does this type punning create
endianness issues on big-endian systems?

> +		else if (page[0] == '-')
> +			vkms_config_plane_remove_format(plane->config, *(int *)fmt);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return count;
> +}

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