Re: [PATCH v5 22/38] drm/vkms: Introduce config for plane format

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Format string vulnerability when initializing planes due to user-controlled plane name.
- [Medium] vkms_config_is_valid() does not verify that a plane has at least one supported format.
- [Medium] Leaked plane configuration on configfs directory creation failure causes permanent device validation failure.
--

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

drm/vkms: Introduce config for plane format

VKMS driver supports all the pixel formats for planes, but for testing it
can be useful to only advertise few of them. This new configuration
interface will allow configuring the pixel format per planes.

> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 5538716467617..c7de3e3073674 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
[ ... ]
> @@ -658,6 +712,80 @@ static struct vkms_config_plane *vkms_config_crtc_get_plane(const struct vkms_co
>  	return NULL;
>  }
>  
> +int __must_check vkms_config_plane_add_all_formats(struct vkms_config_plane *plane_cfg)
> +{
[ ... ]
> +void vkms_config_plane_remove_all_formats(struct vkms_config_plane *plane_cfg)
> +{
> +	plane_cfg->supported_formats_count = 0;
> +	kfree(plane_cfg->supported_formats);
> +	plane_cfg->supported_formats = NULL;
> +}

[Severity: Medium]
Since we can now remove all formats, shouldn't vkms_config_is_valid() verify
that a plane has at least one supported format? 

If a plane has 0 supported formats, drm_universal_plane_init() will trigger a
WARN_ON and fail the initialization when the device is enabled.

> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index d4ccf418198e8..6e767cdf81942 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
[ ... ]
> @@ -227,7 +191,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  
>  	plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
>  					   &vkms_plane_funcs,
> -					   vkms_formats, ARRAY_SIZE(vkms_formats),
> +					   vkms_config_plane_get_supported_formats(plane_cfg),
> +					   vkms_config_plane_get_supported_formats_count(plane_cfg),
>  					   NULL, vkms_config_plane_get_type(plane_cfg),
>  					   vkms_config_plane_get_name(plane_cfg));

[Severity: High]
Does this code introduce a format string vulnerability?

The user-controlled plane name is passed directly as the format string
argument to drmm_universal_plane_alloc() without a "%s" format specifier.
If userspace passes a name containing format specifiers (e.g. "%s%x"),
this could cause an out-of-bounds memory read or kernel crash when
kvasprintf() is called.

While reviewing these changes, I noticed a related issue in make_plane_group()
in drivers/gpu/drm/vkms/vkms_configfs.c:

drivers/gpu/drm/vkms/vkms_configfs.c:make_plane_group()
[ ... ]
		ret = vkms_config_plane_set_name(plane->config, name, strlen(name));
		if (ret) {
			kfree(plane);
			return ERR_PTR(ret);
		}
[ ... ]

[Severity: Medium]
Does this error path leak the plane configuration?

If setting the plane name fails, the configfs object plane is freed but it
fails to call vkms_config_destroy_plane(plane->config). This leaves the
unusable configuration object permanently attached to the device list,
causing vkms_config_is_valid() to unconditionally fail later.

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