Re: [PATCH v4 04/10] drm/appletbdrm: Allocate request/response buffers in begin_fb_access

[sashiko-bot@kernel.org]

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 <tzimmermann@suse.de>

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/20260530185716.65688-1-tzimmermann@suse.de?part=4