Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] Using the new plane state's unclipped `dst` coordinates in `atomic_disable` fails to clear the actual old plane area and allows arbitrary out-of-bounds memory overwrites. -- --- Patch [1]: [PATCH RESEND] drm/sysfb: Fix atomic_disable to use dst coordinates instead of src --- commit 7865516b4b393e248319c8544570c784fcc3881b Author: [email protected] <[email protected]> drm/sysfb: Fix atomic_disable to use dst coordinates instead of src This commit modifies drm_sysfb_plane_helper_atomic_disable() to use the plane_state->dst coordinates rather than the plane_state->src_* coordinates for constructing the clear rectangle. > diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c > b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c > index d2de29caf89eb..18466de886aaa 100644 > --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c > +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c > @@ -380,14 +380,10 @@ void drm_sysfb_plane_helper_atomic_disable(struct > drm_plane *plane, > void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping > abstraction */ > unsigned int dst_pitch = sysfb->fb_pitch; > const struct drm_format_info *dst_format = sysfb->fb_format; > - struct drm_rect dst_clip; > + struct drm_rect dst_clip = plane_state->dst; > unsigned long lines, linepixels, i; > int idx; > > - drm_rect_init(&dst_clip, > - plane_state->src_x >> 16, plane_state->src_y >> 16, > - plane_state->src_w >> 16, plane_state->src_h >> 16); > - > lines = drm_rect_height(&dst_clip); > linepixels = drm_rect_width(&dst_clip); [Severity: Critical] This isn't a bug introduced by this patch, but can this lead to an out-of-bounds memory overwrite when userspace provides unclipped coordinates? During a plane disable (FB_ID = 0), drm_atomic_helper_check_plane_state() populates plane_state->dst with raw, unclipped userspace values because the framebuffer is NULL. If userspace passes negative or massive coordinates for CRTC_X and CRTC_Y, this unclipped dst rectangle is passed to drm_fb_clip_offset(), resulting in a large out-of-bounds pointer offset. The driver then executes memset_io() over this invalid offset later in the function: dst_vmap += drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip); for (i = 0; i < lines; ++i) { memset_io(dst_vmap, 0, linepixels * dst_format->cpp[0]); Should the disable path use the old plane state rather than the new plane state to safely clear the actual old plane area? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
