Re: [PATCH] drm/vkms: Fix gamma_lut size check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the vkms_composer_worker accesses the currently committed CRTC state (crtc->state) asynchronously without locks, leading to a Time-of-Check to Time-of-Use (TOCTOU) race condition and Use-After-Free (UAF).
--

--- Patch [1]: [PATCH] drm/vkms: Fix gamma_lut size check ---
commit a52b4df7305f28acb171652e76fbc632383dbfb6
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Fix gamma_lut size check
    
The gamma_lut size check in vkms_atomic_check() uses
sizeof(struct drm_color_lut *) instead of sizeof(struct drm_color_lut).
On 64-bit both happen to be 8 bytes, so the check works by accident.
On 32-bit sizeof(pointer) is 4 bytes while sizeof(struct drm_color_lut)
is 8 bytes, which would incorrectly reject valid 256-entry LUTs.

> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 5a640b531d88f..2db3971f72c31 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -112,7 +112,7 @@ static int vkms_atomic_check(struct drm_device *dev, struct drm_atomic_commit *s
>  		if (!new_crtc_state->gamma_lut || !new_crtc_state->color_mgmt_changed)
>  			continue;
>  
> -		if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut *)
> +		if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut)
>  		    > VKMS_LUT_SIZE)
>  			return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but since we are looking at the
gamma_lut, is it possible for vkms_composer_worker() to hit a use-after-free
when accessing the LUT blob data?

In drivers/gpu/drm/vkms/vkms_composer.c:vkms_composer_worker():

	if (crtc->state->gamma_lut) {
		s64 max_lut_index_fp;
		s64 u16_max_fp = drm_int2fixp(0xffff);

		crtc_state->gamma_lut.base = (struct drm_color_lut *)crtc->state->gamma_lut->data;
		crtc_state->gamma_lut.lut_length =
			crtc->state->gamma_lut->length / sizeof(struct drm_color_lut);

The worker accesses the currently committed CRTC state (crtc->state)
asynchronously without locks, rather than using its own atomic state snapshot.

If userspace rapidly issues atomic commits that update or remove the CRTC's
gamma LUT, couldn't a concurrent atomic commit swap crtc->state and free
the LUT blob while the worker is executing?

This seems like it could create a large use-after-free window during the
blending phase in compose_active_planes(), or a direct NULL pointer
dereference in the setup phase.

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820083123.544655-2-oushixiong1025@163.com?part=1