Re: [PATCH v5 24/38] drm/vkms: Properly render plane using their zpos

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Heap Out-of-bounds Write / NULL Pointer Dereference in vkms_crtc_atomic_check and blend.
- [High] Validation Bypass / Incorrect API Usage via calling drm_atomic_normalize_zpos in CRTC atomic check.
--

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

drm/vkms: Properly render plane using their zpos

Currently planes are rendered in creation order. This is not an issue, but
with the introduction of new zpos configuration, it is required to
properly render planes.

> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 079abfba427d0..b9428de2d4aa8 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -142,14 +142,20 @@ static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
>  		return -ENOMEM;
>  	vkms_state->num_active_planes = i;
>  
> -	i = 0;
> +	ret = drm_atomic_normalize_zpos(crtc->dev, state);
> +	if (ret)
> +		return ret;

[Severity: High]
Does calling drm_atomic_normalize_zpos() inside a per-CRTC atomic check bypass
plane validation?

In the DRM atomic framework, drm_atomic_helper_check_planes() calls plane
atomic_check() callbacks before CRTC atomic_check() callbacks. By calling
drm_atomic_normalize_zpos() here and adding affected planes to the state, any
planes pulled into the state at this stage will completely miss their
plane-level validation.

Also, since drm_atomic_normalize_zpos() operates on the global atomic state, it
will be executed redundantly for every modified CRTC.

Could this be handled by setting dev->mode_config.normalize_zpos = true during
driver initialization instead, letting the core run the helper before the plane
validation phase?

> +
>  	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;
>  
> -		vkms_state->active_planes[i++] =
> +		// 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);

[Severity: High]
Can this cause an out-of-bounds write if there are invisible planes?

Earlier in this function, vkms_state->active_planes is allocated based on the
number of visible planes (the variable i).

However, drm_atomic_normalize_zpos() computes a sequential normalized_zpos for
all planes attached to the CRTC, not just visible ones. If a userspace client
configures planes such that some are invisible (e.g., clipped or scaled to 0),
the maximum normalized_zpos will exceed the allocated size (i - 1), resulting
in a heap out-of-bounds write.

Additionally, this sparse assignment leaves holes in the array. During
composition, blend() in drivers/gpu/drm/vkms/vkms_composer.c iterates up to
num_active_planes and blindly dereferences the plane array elements. Will this
cause a NULL pointer dereference for the skipped indices?

>  	}
>

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