Re: [PATCH] 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] In `drm_sysfb_plane_helper_atomic_disable()`, using the new plane state's `dst` rectangle allows an attacker to supply arbitrary, unclipped coordinates, leading to out-of-bounds writes of zeroes in kernel or I/O memory.
--

--- Patch [1]: [PATCH] drm/sysfb: Fix atomic_disable to use dst coordinates instead of src ---
commit b1c860517627fd82ea2b305a0494871f248ccba6
Author: [email protected] <[email protected]>

drm/sysfb: Fix atomic_disable to use dst coordinates instead of src

This patch updates drm_sysfb_plane_helper_atomic_disable() to use the 
plane_state->dst coordinates for constructing the clear rectangle. This 
makes the behavior consistent with drm_sysfb_plane_helper_atomic_update() 
which already uses the destination rectangle computed by the DRM core.

> 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;

[Severity: Critical]
This is a pre-existing issue, but does using the new plane state's dst
rectangle allow userspace to supply arbitrary, unclipped coordinates?

If userspace with DRM_MASTER privilege issues an atomic modeset with FB_ID = 0
(disabling the plane) while supplying arbitrary CRTC_X and CRTC_Y properties,
drm_atomic_helper_check_plane_state() detects !fb and returns early. This
bypasses coordinate bounds checking and clipping, leaving plane_state->dst
set to the unclipped, userspace-controlled values.

Then, drm_sysfb_plane_helper_atomic_disable() calculates a memory offset using
drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip), which translates directly
to the userspace-supplied coordinates. This offset is used in a loop calling
memset_io(), which could lead to out-of-bounds physical or I/O memory writes
of zeroes.

>  	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);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1