Re: [PATCH v3 22/40] drm/virtio: Convert to atomic_create_state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Structural memory under-allocation for subclassed plane state. The virtio-gpu driver subclasses `drm_plane_state` into `virtio_gpu_plane_state` (adding a `fence` member), but uses the generic `drm_atomic_helper_plane_create_state` helper for its `.atomic_create_state` hook. This helper allocates only `sizeof(struct drm_plane_state)`. Consequently, the initial plane state is under-allocated. Although current subclass field accesses (like `fence`) are protected by `if (state->fb)` which prevents an immediate out-of-bounds access on the initial state, this is a strict violation of DRM atomic API contracts which mandate custom allocation hooks when subclassing states.
--

commit 021c0051fe694d9f32c7c2b9fb493bbd17a34379
Author: Maxime Ripard <mripard@kernel.org>

drm/virtio: Convert to atomic_create_state

The plane only initializes a pristine state in its reset hook using
drm_atomic_helper_plane_reset(), which is equivalent to what
atomic_create_state expects. Convert to it.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 1d1b27ece62a7..52971864db50b 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -91,7 +91,7 @@ drm_plane_state *virtio_gpu_plane_duplicate_state(struct drm_plane *plane)
>  static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
>  	.update_plane		= drm_atomic_helper_update_plane,
>  	.disable_plane		= drm_atomic_helper_disable_plane,
> -	.reset			= drm_atomic_helper_plane_reset,
> +	.atomic_create_state = drm_atomic_helper_plane_create_state,

[Severity: Medium]
This is a pre-existing issue, but does assigning the generic helper here result
in structural memory under-allocation for the subclassed plane state?

The virtio-gpu driver subclasses drm_plane_state into virtio_gpu_plane_state
which adds a fence member:

drivers/gpu/drm/virtio/virtgpu_drv.h:
struct virtio_gpu_plane_state {
	struct drm_plane_state base;
	struct virtio_gpu_fence *fence;
};

However, the generic helper drm_atomic_helper_plane_create_state in
drivers/gpu/drm/drm_atomic_state_helper.c only allocates the size of the base
struct drm_plane_state:

drm_atomic_helper_plane_create_state() {
	...
	state = kzalloc_obj(*state);
	...
}

Any subsequent downcast on this initial state treats it as a larger object.
Currently, no out-of-bounds access occurs because accesses to the subclassed
fields (e.g., fence) are guarded by if (state->fb), and the initial state has a
NULL framebuffer.

Should a custom allocation hook be used here to allocate the full
virtio_gpu_plane_state and adhere to the DRM atomic API contracts?

>  	.atomic_duplicate_state = virtio_gpu_plane_duplicate_state,
>  	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-drm-no-more-plane-reset-v3-0-1877c7aa57b3@kernel.org?part=22