Re: [PATCH] drm/fbdev-generic: fix potential out-of-bounds access

[Daniel Vetter]

On Sun, Apr 09, 2023 at 09:21:10PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> 
> We should setting the screen buffer size according to the screen's actual
> size, rather than the size of the GEM object backing the front framebuffer.
> The size of GEM buffer is page size aligned, while the size of active area
> of a specific screen is *NOT* necessarily page size aliged. For example,
> 1680x1050, 1600x900, 1440x900, 800x6000 etc. In those case, the damage rect
> computed by drm_fb_helper_memory_range_to_clip() goes out of bottom bounds
> of the display.
> 
> Run fbdev test of IGT on a x86+ast2400 platform with 1680x1050 resolution
> will cause the system hang with the following call trace:
> 
>   Oops: 0000 [#1] PREEMPT SMP PTI
>   [IGT] fbdev: starting subtest eof
>   Workqueue: events drm_fb_helper_damage_work [drm_kms_helper]
>   [IGT] fbdev: starting subtest nullptr
> 
>   RIP: 0010:memcpy_erms+0xa/0x20
>   RSP: 0018:ffffa17d40167d98 EFLAGS: 00010246
>   RAX: ffffa17d4eb7fa80 RBX: ffffa17d40e0aa80 RCX: 00000000000014c0
>   RDX: 0000000000001a40 RSI: ffffa17d40e0b000 RDI: ffffa17d4eb80000
>   RBP: ffffa17d40167e20 R08: 0000000000000000 R09: ffff89522ecff8c0
>   R10: ffffa17d4e4c5000 R11: 0000000000000000 R12: ffffa17d4eb7fa80
>   R13: 0000000000001a40 R14: 000000000000041a R15: ffffa17d40167e30
>   FS:  0000000000000000(0000) GS:ffff895257380000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: ffffa17d40e0b000 CR3: 00000001eaeca006 CR4: 00000000001706e0
>   Call Trace:
>    <TASK>
>    ? drm_fbdev_generic_helper_fb_dirty+0x207/0x330 [drm_kms_helper]
>    drm_fb_helper_damage_work+0x8f/0x170 [drm_kms_helper]
>    process_one_work+0x21f/0x430
>    worker_thread+0x4e/0x3c0
>    ? __pfx_worker_thread+0x10/0x10
>    kthread+0xf4/0x120
>    ? __pfx_kthread+0x10/0x10
>    ret_from_fork+0x2c/0x50
>    </TASK>
>   CR2: ffffa17d40e0b000
>   ---[ end trace 0000000000000000 ]---
> 
> We also add trival code in this patch to restrict the damage rect beyond
> the last line of the framebuffer.

Nice catch!

> 
> Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_fb_helper.c     | 2 +-
>  drivers/gpu/drm/drm_fbdev_generic.c | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 64458982be40..a2b749372759 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -645,7 +645,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
>  	u32 x1 = 0;
>  	u32 y1 = off / info->fix.line_length;
>  	u32 x2 = info->var.xres;
> -	u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> +	u32 y2 = min_t(u32, DIV_ROUND_UP(end, info->fix.line_length), info->var.yres);

So for additional robustness I think it'd be good if we change the entire
computation here to use drm_framebuffer data and not fb_info data, because
fundamentally that's what the drm kms code consumes. It should all match
anyway, but I think it makes the code more obviously correct.

So in the entire function instead of looking at fb_info->fix we should
probably look at

	struct drm_fb_helper *helper = info->par;

And then helper->fb->pitches[0] and helper->fb->height.

If you agree would be great if you can please respin with that (and the
commit message augmented to explain why we do the change)?
>  
>  	if ((y2 - y1) == 1) {
>  		/*
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index 8e5148bf40bb..a6daecb5f640 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -95,6 +95,8 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
>  	fb_helper->fb = buffer->fb;
>  
>  	screen_size = buffer->gem->size;

I guess you forgot to remove this line here? Also I'm not understanding
why this matters, I think you're fix only needs the above chunk, not this
one? If I got this right then please drop this part, there's drivers which
only use drm_fb_helper.c but not drm_fbdev_generic.c, and from what I can
tell they all still set the gem buffer size here.

If otoh we need this too, then there's a few more places that need to be
fixed.

> +	screen_size = sizes->surface_height * buffer->fb->pitches[0];
> +
>  	screen_buffer = vzalloc(screen_size);
>  	if (!screen_buffer) {
>  		ret = -ENOMEM;

Cheers, Daniel

> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch