[PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
Thomas Zimmermann
tzimmermann at suse.de
Thu Apr 13 19:16:34 UTC 2023
Hi,
thanks for the patch. This is effectively a revert of commit
8fbc9af55de0 ("drm/fbdev-generic: Set screen size to size of GEM
buffer"). Please add a Fixes tag.
Am 13.04.23 um 20:06 schrieb Sui Jingfeng:
> From: Sui Jingfeng <suijingfeng at loongson.cn>
>
> The crazy fbdev test of IGT may write after EOF, which lead to out-of-bound
Please drop 'crazy'. :)
> access for the drm drivers using fbdev-generic. For example, run fbdev test
> on a x86-64+ast2400 platform with 1680x1050 resolution will cause the linux
> kernel hang with 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 ]---
>
> The indirect reason is drm_fb_helper_memory_range_to_clip() generate damage
> rectangles which partially or completely go out of the active display area.
> The second of argument 'off' is passing from the user-space, this will lead
> to the out-of-bound if it is large than (fb_height + 1) * fb_pitches; while
> DIV_ROUND_UP() may also controbute to error by 1.
>
> This patch will add code to restrict the damage rect computed go beyond of
> the last line of the framebuffer.
>
> Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
> drivers/gpu/drm/drm_fbdev_generic.c | 2 +-
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 64458982be40..6bb1b8b27d7a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -641,19 +641,27 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off, size_t len,
> struct drm_rect *clip)
> {
> + u32 line_length = info->fix.line_length;
> + u32 fb_height = info->var.yres;
> off_t end = off + len;
> u32 x1 = 0;
> - u32 y1 = off / info->fix.line_length;
> + u32 y1 = off / line_length;
> u32 x2 = info->var.xres;
> - u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
> + u32 y2 = DIV_ROUND_UP(end, line_length);
> +
> + /* Don't allow any of them beyond the bottom bound of display area */
> + if (y1 > fb_height)
> + y1 = fb_height;
> + if (y2 > fb_height)
> + y2 = fb_height;
>
> if ((y2 - y1) == 1) {
> /*
> * We've only written to a single scanline. Try to reduce
> * the number of horizontal pixels that need an update.
> */
> - off_t bit_off = (off % info->fix.line_length) * 8;
> - off_t bit_end = (end % info->fix.line_length) * 8;
> + off_t bit_off = (off % line_length) * 8;
> + off_t bit_end = (end % line_length) * 8;
Please scratch all these changes. The current code should work as
intended. Only the generic fbdev emulation uses this code and it should
really be moved there at some point.
>
> x1 = bit_off / info->var.bits_per_pixel;
> x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index 8e5148bf40bb..b057cfbba938 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -94,7 +94,7 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
> fb_helper->buffer = buffer;
> fb_helper->fb = buffer->fb;
>
> - screen_size = buffer->gem->size;
> + screen_size = sizes->surface_height * buffer->fb->pitches[0];
I guess we simply go back to this line. I'd R-b a patch that does
exactly this.
But some explanation is in order. Maybe you can add this as a comment to
the computation, as it's not obvious:
The value of screen_size should actually be the size of the gem buffer.
In a physical framebuffer (i.e., video memory), the size would be a
multiple of the page size, but not necessarily a multiple of the screen
resolution. There are also pan fbdev's operations, and we could possibly
use DRM buffers that are not multiples of the screen width. But the
update code requires the use of drm_framebuffer_funcs.dirty, which takes
a clipping rectangle and therefore doesn't work well with these odd
values for screen_size.
Best regards
Thomas
> screen_buffer = vzalloc(screen_size);
> if (!screen_buffer) {
> ret = -ENOMEM;
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230413/1a6403fe/attachment.sig>
More information about the dri-devel
mailing list