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
