Re: [PATCH v3 05/10] drm/appletbdrm: Allocate request/response buffers in begin_fb_access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unbounded accumulation of damage clip sizes causes an integer overflow, resulting in an undersized buffer allocation and subsequent out-of-bounds heap write.
- [High] The plane `atomic_update` callback incorrectly calls sleeping functions, violating DRM atomic context guidelines.
- [High] Mismatched coordinate space intersection and inconsistent calculation ordering lead to uninitialized memory gaps and malformed hardware payloads.
--

commit fc37b98d4bfc3183056fa690c4e2e659894c67e7
Author: Thomas Zimmermann <tzimmermann@suse.de>
Date:   Wed May 27 14:46:24 2026 +0000

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_FORMAT_XRGB8888, /* emulated */
>  };
>  
> -static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						   struct drm_atomic_commit *state)
> +static int appletbdrm_primary_plane_helper_begin_fb_access(struct drm_plane *plane,
> +							   struct drm_plane_state *new_plane_state)
>  {
>  	struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(new_plane_state);
> +	size_t frames_size = 0;
>  	struct drm_atomic_helper_damage_iter iter;
>  	struct drm_rect damage;
>  	size_t request_size;
>  
> -	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, new_plane_state);
> +	drm_atomic_helper_damage_iter_init(&iter, NULL, new_plane_state);
>  	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 could the unbounded accumulation of damage
clip sizes cause an integer overflow?

The FB_DAMAGE_CLIPS property allows userspace to pass an unbounded number of
damage clips, limited only by the maximum DRM property blob size.

If millions of clips are provided, accumulating the sizes with:

    frames_size += struct_size(...)

could wrap around size_t on 32-bit systems. Even if frames_size doesn't wrap,
the subsequent calculation:

    request_size = ALIGN(sizeof(...) + frames_size + sizeof(...), 16)

might easily wrap around to a small value, causing kvzalloc() to successfully
allocate an undersized buffer.

When appletbdrm_flush_damage() iteratively processes the exact same clips,
could it trust the per-clip buf_size and copy pixel data into the undersized
buffer via drm_fb_memcpy(), resulting in an out-of-bounds heap write?

[Severity: High]
This is also a pre-existing issue, but does this allocation properly account
for the intersection calculations that happen later?

In appletbdrm_flush_damage(), buf_size is calculated before the intersection:

    buf_size = rect_size(&damage);
    if (!drm_rect_intersect(&dst_clip, &damage))
        continue;

If the intersection shrinks the damage rectangle, drm_fb_memcpy() copies fewer
bytes than buf_size, leaving zeroed memory at the end of the frame buffer.
Since frame->buf_size was already set to the original size, the hardware will
read these zeroes as pixel data.

Furthermore, dst_clip is in CRTC coordinates while damage is in Framebuffer
(SRC) coordinates. If userspace utilizes a framebuffer larger than the CRTC
and applies a source offset, the coordinate spaces will not overlap properly.
This causes valid damage clips to be skipped entirely.

Since the begin_fb_access() helper factored these skipped clips into the total
frames_size allocation without performing the intersection, skipping them later
fails to advance the frame pointer.

Could this leave uninitialized gaps before the footer, corrupting the packed
payload structure sent to the hardware?

[ ... ]

> @@ -468,7 +474,7 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>  }
>  
>  static void appletbdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
> -						     struct drm_atomic_commit *old_state)
> +							  struct drm_atomic_commit *old_state)
>  {
>  	struct appletbdrm_device *adev = drm_to_adev(plane->dev);
>  	struct drm_device *drm = plane->dev;

[Severity: High]
This is a pre-existing issue, but does this code execute sleeping functions
from an atomic context?

According to DRM atomic context guidelines, plane atomic update callbacks
execute in an atomic context where sleeping is forbidden.

The appletbdrm_primary_plane_helper_atomic_update() callback invokes
appletbdrm_flush_damage(), which subsequently calls:

    drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);

This function takes locks and can wait on fences. The flush function then
performs synchronous USB bulk transfers, which also sleep.

Could this break atomic context rules and cause system instability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527145113.241595-1-tzimmermann@suse.de?part=5