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

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

[oushixiong1025]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]


From: Shixiong Ou <oushixiong@kylinos.cn>

drm_sysfb_plane_helper_atomic_disable() constructs the clear rectangle
from plane_state->src_* coordinates. This is inconsistent with
drm_sysfb_plane_helper_atomic_update() which uses plane_state->dst
— the destination rectangle already computed by the DRM core with panel
orientation applied.

Use plane_state->dst directly, matching the approach in atomic_update.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
index d2de29caf89e..18466de886aa 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);
 
-- 
2.25.1


[-- Attachment #2: Type: text/plain, Size: 82 bytes --]

Content-type: Text/plain

No virus found
		Checked by Hillstone Network AntiVirus

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

[sashiko-bot]

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: oushixiong1025@163.com <oushixiong1025@163.com>

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/20260825104117.667943-1-oushixiong1025@163.com?part=1

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

[Thomas Zimmermann]

Hi

Am 25.08.26 um 12:41 schrieb oushixiong1025@163.com:
> From: Shixiong Ou <oushixiong@kylinos.cn>
>
> drm_sysfb_plane_helper_atomic_disable() constructs the clear rectangle
> from plane_state->src_* coordinates. This is inconsistent with
> drm_sysfb_plane_helper_atomic_update() which uses plane_state->dst
> — the destination rectangle already computed by the DRM core with panel
> orientation applied.
>
> Use plane_state->dst directly, matching the approach in atomic_update.

Thanks for bringing this up.  I think the issue needs to be addressed in 
a different way.  The plane's video memory is behind dst_vmap. It needs 
to be cleared to 0 entirely without considering any user-given sizes.  
The relevant size can be found in the fb_mode.

Here's some pseudo code:

   drm_rect_init(dst_clip, 0, 0, sysfb->fb_mode.width, 
sysfb->fb_mode_height).

   lines = drm_rect_height(dst_clip);
   linepixels = drm_rect_width(dst_clip);
   linesize = drm_format_info_min_pitch(sysfb->fb_format, 0, linepixels);

   dst_size = min(linesize, dst_pitch);

   for (i < lines) {
     memset_io(dst_vmap, 0, dst_size);
     dst_vmap += dst_pitch
   }

This should safely clear the visible part of the buffer without touching 
any non-visible bytes at the end of each line. It also does not depend 
on user-controlled size input.

Do you want to provide an updated patch  or shall I send it?

Best regards
Thomas

>
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
>   drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> index d2de29caf89e..18466de886aa 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);
>   
>
> Content-type: Text/plain
>
> No virus found
> 		Checked by Hillstone Network AntiVirus

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)