Re: [PATCH 06/11] drm/vkms: rename struct vkms_color_lut::base to y

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

Pre-existing issues:
- [Critical] vkms_composer_worker accesses the global crtc->state asynchronously, creating a Use-After-Free (UAF) vulnerability.
- [High] vkms_atomic_check incorrectly divides gamma_lut length by sizeof(pointer) instead of sizeof(struct) on 32-bit systems.
--

--- Patch [6]: [PATCH 06/11] drm/vkms: rename struct vkms_color_lut::base to y ---
commit 89753bc7023e7c4dac9293962b90fb28fe7958fd
Author: Leandro Ribeiro <[email protected]>

drm/vkms: rename struct vkms_color_lut::base to y

This patch renames the base field in struct vkms_color_lut to y to prepare 
for the addition of an x field in future commits. It updates all usages of 
the field across the driver's tests, composer, and LUT initialization code. 
There is no intended functional change.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c

[ ... ]

> @@ -641,7 +641,7 @@ void vkms_composer_worker(struct work_struct *work)
>  		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.y = (struct drm_color_lut *)crtc->state->gamma_lut->data;

[Severity: Critical]
This isn't a bug introduced by this patch, but does accessing the global
crtc->state asynchronously here create a use-after-free vulnerability?

If userspace submits rapid non-blocking atomic commits, crtc->state can be 
swapped by drm_atomic_helper_commit() and its previous gamma_lut blob freed 
while vkms_composer_worker() is still executing.

Could this lead to the worker dereferencing the freed gamma_lut->data pointer 
later in apply_lut_to_channel_value()?

>  		crtc_state->gamma_lut.lut_length =
>  			crtc->state->gamma_lut->length / sizeof(struct drm_color_lut);
>  		max_lut_index_fp = drm_int2fixp(crtc_state->gamma_lut.lut_length - 1);

[ ... ]

While reviewing the gamma LUT handling, I also noticed the following in
vkms_drv.c:vkms_atomic_check():

> 		if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut *)
> 		    > VKMS_GAMMA_LUT_SIZE)
> 			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does vkms_atomic_check() incorrectly divide 
by sizeof(struct drm_color_lut *) instead of sizeof(struct drm_color_lut)?

On 32-bit systems where pointers are 4 bytes, a 256-entry LUT evaluates to a 
length of 512, which exceeds VKMS_GAMMA_LUT_SIZE (256) and rejects valid 
atomic commits with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6