Re: [PATCH v5 14/38] drm/vkms: Use plane folder name as plane name

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] User-provided plane name from configfs is used directly as a format string in `drmm_universal_plane_alloc()`.
- [Medium] Missing cleanup of `plane->config` on the `vkms_config_plane_set_name()` error path in `make_plane_group()`.
- [Medium] `drm_get_rotation_name` incorrectly uses a rotation mask as an array index, contrary to its documentation.
--

commit 71fdf3bbf310c477150a383e21a3c104a2d6795c
Author: Louis Chauvet <louis.chauvet@bootlin.com>
Subject: drm/vkms: Use plane folder name as plane name

The plane name is now directly derived from the configfs folder name. When a
plane directory is created under /config/vkms/<device>/planes/, the folder
name is used as the plane name via vkms_config_plane_set_name.

> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 97f87a53d202a..0744ac40814d8 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -377,6 +377,12 @@ static struct config_group *make_plane_group(struct config_group *group,
>  			return ERR_PTR(ret);
>  		}
>  
> +		ret = vkms_config_plane_set_name(plane->config, name, strlen(name));

[Severity: High]
Could this introduce a format string regression?

Since this user-provided configfs folder name is stored as the plane name,
it appears it gets passed directly as the format string parameter to
drmm_universal_plane_alloc() later during initialization:

drivers/gpu/drm/vkms/vkms_plane.c:vkms_plane_init()
    plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
        &vkms_plane_funcs,
        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));

If a user creates a configfs directory with a name containing printf-style
format specifiers (e.g., "%s%n"), can it lead to a kernel crash or memory
corruption?

> +		if (ret) {
> +			kfree(plane);

[Severity: Medium]
Is there a regression in this error path where it leaves a dangling plane
configuration?

If vkms_config_plane_set_name() fails (e.g., due to memory allocation
failure), this calls kfree(plane). However, it seems plane->config was
already appended to dev->config->planes prior to this, but we don't call
vkms_config_destroy_plane(plane->config) here.

Can this leave a ghost plane in the device configuration that lacks a
corresponding configfs directory?

> +			return ERR_PTR(ret);
> +		}
> +
>  		config_group_init_type_name(&plane->group, name, &plane_item_type);
>  
>  		config_group_init_type_name(&plane->possible_crtcs_group,

[Severity: Medium]
Will the following logic in drm_get_rotation_name() cause a regression by
incorrectly using a rotation mask as an array index?

In drivers/gpu/drm/drm_blend.c:drm_get_rotation_name():

    const char *drm_get_rotation_name(unsigned int rotation)
    {
        if (rotation < ARRAY_SIZE(rotation_props))
            return rotation_props[rotation].name;

        return "(unknown)";
    }

The documentation for this function explicitly states the rotation parameter
expects a rotation mask (DRM_MODE_ROTATE_* | DRM_MODE_REFLECT_*). However,
the implementation expects an array index (0-5) and incorrectly uses the
mask as an index.

For example, passing the mask DRM_MODE_ROTATE_90 (which is 2) returns
"rotate-180" (the value at index 2).

Can this cause incorrect strings to be returned to new drivers or core code
using this exported function?

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