Re: [PATCH v5 26/38] drm/vkms: Introduce configfs for plane zpos property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Heap buffer overflow in `vkms_crtc_atomic_check` due to array indexing with `normalized_zpos` exceeding allocated size based on visible planes.
- [High] Memory leak and ghost object creation in `make_plane_group` if `vkms_config_plane_set_name` fails.
- [High] Unaligned memory access when parsing fourcc formats in `plane_supported_formats_store`.
--

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

drm/vkms: Introduce configfs for plane zpos property

This commit introduces configfs attributes for managing the zpos property
of VKMS planes. It adds attributes to configure whether zpos is enabled,
mutable, its initial value, and its min/max boundaries.

> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index e6cb245b8582d..95f4594fb683d 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -687,6 +687,176 @@ static ssize_t plane_supported_formats_store(struct config_item *item,
>  	return count;
>  }
>  

[Severity: High]
This isn't a bug introduced by this patch, but can this unaligned memory
access in plane_supported_formats_store() cause a kernel panic on strict
alignment architectures?

When userspace writes a new format string, the input is copied into a
4-byte char array on the stack, which is then cast to an int pointer and
dereferenced:

drivers/gpu/drm/vkms/vkms_configfs.c:plane_supported_formats_store() {
	...
	scoped_guard(mutex, &plane->dev->lock) {
		char fmt[4] = {' ', ' ', ' ', ' '};

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

Since a char array only guarantees 1-byte alignment, the 32-bit dereference
can trigger an unaligned access exception.

[Severity: Critical]
This is a pre-existing issue, but does this array indexing overflow the
active_planes array in vkms_crtc_atomic_check()?

The active_planes array is allocated with a size equal to the number of
strictly visible planes:

drivers/gpu/drm/vkms/vkms_crtc.c:vkms_crtc_atomic_check() {
	...
	drm_for_each_plane_mask(plane, crtc->dev, crtc_state->plane_mask) {
		plane_state = drm_atomic_get_new_plane_state(crtc_state->state, plane);
		WARN_ON(!plane_state);

		if (!plane_state->visible)
			continue;
		i++;
	}

	vkms_state->active_planes = kzalloc_objs(*vkms_state->active_planes, i);
	...

However, the array is populated using normalized_zpos as the index:

	...
	drm_for_each_plane_mask(plane, crtc->dev, crtc_state->plane_mask) {
		plane_state = drm_atomic_get_new_plane_state(crtc_state->state, plane);

		if (!plane_state->visible)
			continue;

		// Order planes according to their normalized_zpos
		// After drm_atomic_normalize_zpos, the possible values are 0..n_planes-1
		// so store them using this index
		vkms_state->active_planes[plane_state->normalized_zpos] =
			to_vkms_plane_state(plane_state);
	}
	...
}

If a user configures some planes as invisible but gives a visible plane a
higher zpos than the invisible ones, drm_atomic_normalize_zpos() will assign
the visible plane a normalized_zpos value greater than or equal to the total
number of strictly visible planes, resulting in an out-of-bounds write.

[Severity: High]
This is a pre-existing issue, but does make_plane_group() leak the
internal vkms_config_plane object and create a ghost plane if setting
the name fails?

drivers/gpu/drm/vkms/vkms_configfs.c:make_plane_group() {
	...
		plane->config = vkms_config_create_plane(dev->config);
		if (IS_ERR(plane->config)) {
			ret = PTR_ERR(plane->config);
			kfree(plane);
			return ERR_PTR(ret);
		}

		ret = vkms_config_plane_set_name(plane->config, name, strlen(name));
		if (ret) {
			kfree(plane);
			return ERR_PTR(ret);
		}
	...
}

If vkms_config_plane_set_name() returns an error, the configfs wrapper
is freed, but vkms_config_destroy_plane(plane->config) is not called.

The internal object remains in dev->config->planes. Because it will never
have a CRTC attached, vkms_config_is_valid() will always fail, permanently
preventing the device from being enabled.

[ ... ]

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