Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] Passing `NULL` as the `old_state` parameter to `drm_atomic_helper_damage_iter_init()` causes a kernel panic due to a NULL pointer dereference. Pre-existing issues: - [Critical] Accumulating damage clip sizes in `frames_size` can overflow on 32-bit platforms, causing an undersized buffer allocation and a subsequent heap buffer overflow. -- commit 72afced3e1b6baa294269874c7c4d9d05230889c Author: Thomas Zimmermann <[email protected]> drm/appletbdrm: Allocate request/response buffers in begin_fb_access In atomic_check, damage handling is not fully evaluated. Another atomic_check helper could trigger a full modeset and thus invalidate damage clips. Allocation of the request/response buffers in appletbdrm depends on correct damage information. Otherwise it might allocate incorrectly sized buffers. Allocate the buffers in the driver's begin_fb_access helper. It runs early during the commit when damage clipping has been fully evaluated. > diff --git a/drivers/gpu/drm/tiny/appletbdrm.c > b/drivers/gpu/drm/tiny/appletbdrm.c > index cdd35af498920..b683dcb35b0be 100644 > --- a/drivers/gpu/drm/tiny/appletbdrm.c > +++ b/drivers/gpu/drm/tiny/appletbdrm.c > @@ -315,33 +315,16 @@ static const u32 appletbdrm_primary_plane_formats[] = { [ ... ] > - drm_atomic_helper_damage_iter_init(&iter, old_plane_state, > new_plane_state); > + drm_atomic_helper_damage_iter_init(&iter, NULL, new_plane_state); [Severity: Critical] Does passing NULL as the old_state parameter to drm_atomic_helper_damage_iter_init() cause a kernel panic? If userspace has provided damage clips, drm_atomic_helper_damage_iter_init() evaluates !drm_rect_equals(&state->src, &old_state->src) to check if the source coordinates changed: drivers/gpu/drm/drm_damage_helper.c:drm_atomic_helper_damage_iter_init() { ... if (!iter->clips || state->ignore_damage_clips || !drm_rect_equals(&state->src, &old_state->src)) { ... } This appears to pass the address of src offset from the NULL pointer to drm_rect_equals(), which would immediately dereference it and trigger an oops. This could allow any userspace process with display access to trivially crash the system by attaching a damage clip. I found the fix for this regression later in the patch series in the commit 'drm/damage-helper: Remove old state from drm_atomic_helper_damage_iter_init()', but I am reporting it here since it breaks at this step in the git history. > drm_atomic_for_each_plane_damage(&iter, &damage) { > frames_size += struct_size((struct appletbdrm_frame *)0, buf, > rect_size(&damage)); [Severity: Critical] This is a pre-existing issue, but can this unchecked accumulation overflow frames_size on 32-bit platforms? Because userspace can submit an arbitrary number of overlapping damage clips up to the maximum property blob size, this unchecked addition could wrap around a 32-bit size_t counter, causing kvzalloc() to allocate an undersized buffer. During appletbdrm_flush_damage(), the driver loops over the same clips and uses drm_fb_memcpy() to write the full, un-overflowed volume of pixel data into the undersized allocation: drivers/gpu/drm/tiny/appletbdrm.c:appletbdrm_flush_damage() { ... switch (fb->format->format) { case DRM_FORMAT_XRGB8888: drm_fb_xrgb8888_to_bgr888(&dst, NULL, &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state); break; default: drm_fb_memcpy(&dst, NULL, &shadow_plane_state->data[0], fb, &damage); break; } ... } This causes an out-of-bounds heap write, which could potentially be exploited. > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
