Re: [PATCH RESEND] drm/sysfb: Fix atomic_disable to use dst coordinates instead of src

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