diff mbox series

[v5] drm/fbdev-generic: prohibit potential out-of-bounds access

Message ID 20230420030500.1578756-1-suijingfeng@loongson.cn
State New
Headers
Series [v5] drm/fbdev-generic: prohibit potential out-of-bounds access |

Commit Message

Sui Jingfeng April 20, 2023, 3:05 a.m. UTC 
  The fbdev test of IGT may write after EOF, which lead to out-of-bound
access for drm drivers hire fbdev-generic. For example, run fbdev test
on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
linux kernel 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 ]---

The is because damage rectangles computed by
drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
bound in the screen's active display area. Possible reasons are:

1) Buffers are allocated in the granularity of page size, for mmap system
   call support. The shadow screen buffer consumed by fbdev emulation may
   also choosed be page size aligned.

2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
   will introduce off-by-one error.

For example, on a 16KB page size system, in order to store a 1920x1080
XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
1920*1080*4 can not be divided exactly by 16KB.

 1920 * 1080 * 4 = 8294400 bytes
 506 * 16 * 1024 = 8290304 bytes
 507 * 16 * 1024 = 8306688 bytes

 line_length = 1920*4 = 7680 bytes

 507 * 16 * 1024 / 7680 = 1081.6

 off / line_length = 507 * 16 * 1024 / 7680 = 1081
 DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082

memcpy_toio() typically issue the copy line by line, when copy the last
line, out-of-bound access will be happen. Because:

 1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688

Note that userspace may stil write to the invisiable area if a larger
buffer than width x stride is exposed. But it is not a big issue as
long as there still have memory resolve the access if not drafting so
far.

 - Also limit the y1 (Daniel)
 - keep fix patch it to minimal (Daniel)
 - screen_size is page size aligned because of it need mmap (Thomas)
 - Adding fixes tag (Thomas)

Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Link: https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
---
 drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Thomas Zimmermann April 20, 2023, 7:04 a.m. UTC | #1 
Hi,

this patch looks to me good and I'd like to merge it, if no one objects.

In the near future, after i915 has switched to struct drm_client, I 
intend to move DRM's deferred-I/O helpers into fbdev-generic and i915. 
Those are the two users, but they are fairly different. They can then 
both have something tailored towards their needs.

Best regards
Thomas

Am 20.04.23 um 05:05 schrieb Sui Jingfeng:
> The fbdev test of IGT may write after EOF, which lead to out-of-bound
> access for drm drivers hire fbdev-generic. For example, run fbdev test
> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
> linux kernel 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 ]---
> 
> The is because damage rectangles computed by
> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
> bound in the screen's active display area. Possible reasons are:
> 
> 1) Buffers are allocated in the granularity of page size, for mmap system
>     call support. The shadow screen buffer consumed by fbdev emulation may
>     also choosed be page size aligned.
> 
> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>     will introduce off-by-one error.
> 
> For example, on a 16KB page size system, in order to store a 1920x1080
> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
> 1920*1080*4 can not be divided exactly by 16KB.
> 
>   1920 * 1080 * 4 = 8294400 bytes
>   506 * 16 * 1024 = 8290304 bytes
>   507 * 16 * 1024 = 8306688 bytes
> 
>   line_length = 1920*4 = 7680 bytes
> 
>   507 * 16 * 1024 / 7680 = 1081.6
> 
>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
> 
> memcpy_toio() typically issue the copy line by line, when copy the last
> line, out-of-bound access will be happen. Because:
> 
>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
> 
> Note that userspace may stil write to the invisiable area if a larger
> buffer than width x stride is exposed. But it is not a big issue as
> long as there still have memory resolve the access if not drafting so
> far.
> 
>   - Also limit the y1 (Daniel)
>   - keep fix patch it to minimal (Daniel)
>   - screen_size is page size aligned because of it need mmap (Thomas)
>   - Adding fixes tag (Thomas)
> 
> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Link: https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 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;
>   
>   		x1 = bit_off / info->var.bits_per_pixel;
>   		x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Thomas Zimmermann April 20, 2023, 7:07 a.m. UTC | #2 
Hi

Am 20.04.23 um 09:04 schrieb Thomas Zimmermann:
> Hi,
> 
> this patch looks to me good and I'd like to merge it, if no one objects.

Rereading it, I might have been too eager. What happened to the setting 
of screen_size = buffer->gem->size ?  It is not relevant?

Best regards
Thomas

> 
> In the near future, after i915 has switched to struct drm_client, I 
> intend to move DRM's deferred-I/O helpers into fbdev-generic and i915. 
> Those are the two users, but they are fairly different. They can then 
> both have something tailored towards their needs.
> 
> Best regards
> Thomas
> 
> Am 20.04.23 um 05:05 schrieb Sui Jingfeng:
>> The fbdev test of IGT may write after EOF, which lead to out-of-bound
>> access for drm drivers hire fbdev-generic. For example, run fbdev test
>> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
>> linux kernel 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 ]---
>>
>> The is because damage rectangles computed by
>> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
>> bound in the screen's active display area. Possible reasons are:
>>
>> 1) Buffers are allocated in the granularity of page size, for mmap system
>>     call support. The shadow screen buffer consumed by fbdev emulation 
>> may
>>     also choosed be page size aligned.
>>
>> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>>     will introduce off-by-one error.
>>
>> For example, on a 16KB page size system, in order to store a 1920x1080
>> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
>> 1920*1080*4 can not be divided exactly by 16KB.
>>
>>   1920 * 1080 * 4 = 8294400 bytes
>>   506 * 16 * 1024 = 8290304 bytes
>>   507 * 16 * 1024 = 8306688 bytes
>>
>>   line_length = 1920*4 = 7680 bytes
>>
>>   507 * 16 * 1024 / 7680 = 1081.6
>>
>>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>>
>> memcpy_toio() typically issue the copy line by line, when copy the last
>> line, out-of-bound access will be happen. Because:
>>
>>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>>
>> Note that userspace may stil write to the invisiable area if a larger
>> buffer than width x stride is exposed. But it is not a big issue as
>> long as there still have memory resolve the access if not drafting so
>> far.
>>
>>   - Also limit the y1 (Daniel)
>>   - keep fix patch it to minimal (Daniel)
>>   - screen_size is page size aligned because of it need mmap (Thomas)
>>   - Adding fixes tag (Thomas)
>>
>> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Link: 
>> https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 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;
>>           x1 = bit_off / info->var.bits_per_pixel;
>>           x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Geert Uytterhoeven April 20, 2023, 9:04 a.m. UTC | #3 
Hi Sui,

On Thu, Apr 20, 2023 at 5:09 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
> The fbdev test of IGT may write after EOF, which lead to out-of-bound
> access for drm drivers hire fbdev-generic. For example, run fbdev test
> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
> linux kernel 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 ]---
>
> The is because damage rectangles computed by
> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
> bound in the screen's active display area. Possible reasons are:
>
> 1) Buffers are allocated in the granularity of page size, for mmap system
>    call support. The shadow screen buffer consumed by fbdev emulation may
>    also choosed be page size aligned.
>
> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>    will introduce off-by-one error.
>
> For example, on a 16KB page size system, in order to store a 1920x1080
> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
> 1920*1080*4 can not be divided exactly by 16KB.
>
>  1920 * 1080 * 4 = 8294400 bytes
>  506 * 16 * 1024 = 8290304 bytes
>  507 * 16 * 1024 = 8306688 bytes
>
>  line_length = 1920*4 = 7680 bytes
>
>  507 * 16 * 1024 / 7680 = 1081.6
>
>  off / line_length = 507 * 16 * 1024 / 7680 = 1081
>  DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>
> memcpy_toio() typically issue the copy line by line, when copy the last
> line, out-of-bound access will be happen. Because:
>
>  1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>
> Note that userspace may stil write to the invisiable area if a larger
> buffer than width x stride is exposed. But it is not a big issue as
> long as there still have memory resolve the access if not drafting so
> far.
>
>  - Also limit the y1 (Daniel)
>  - keep fix patch it to minimal (Daniel)
>  - screen_size is page size aligned because of it need mmap (Thomas)
>  - Adding fixes tag (Thomas)
>
> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks for the update!  This v5 is completely different from the v3
I tested before, so keeping my Tested-by is not really appropriate...

I have retested fbtest with shmob-drm on Armadillo-800-EVA
(800x480@RG16, i.e. 187.5 pages), and fortunately this version still
works fine, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Sui Jingfeng April 20, 2023, 9:31 a.m. UTC | #4 
Hi,

On 2023/4/20 17:04, Geert Uytterhoeven wrote:
> Hi Sui,
>
> On Thu, Apr 20, 2023 at 5:09 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>> The fbdev test of IGT may write after EOF, which lead to out-of-bound
>> access for drm drivers hire fbdev-generic. For example, run fbdev test
>> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
>> linux kernel 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 ]---
>>
>> The is because damage rectangles computed by
>> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
>> bound in the screen's active display area. Possible reasons are:
>>
>> 1) Buffers are allocated in the granularity of page size, for mmap system
>>     call support. The shadow screen buffer consumed by fbdev emulation may
>>     also choosed be page size aligned.
>>
>> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>>     will introduce off-by-one error.
>>
>> For example, on a 16KB page size system, in order to store a 1920x1080
>> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
>> 1920*1080*4 can not be divided exactly by 16KB.
>>
>>   1920 * 1080 * 4 = 8294400 bytes
>>   506 * 16 * 1024 = 8290304 bytes
>>   507 * 16 * 1024 = 8306688 bytes
>>
>>   line_length = 1920*4 = 7680 bytes
>>
>>   507 * 16 * 1024 / 7680 = 1081.6
>>
>>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>>
>> memcpy_toio() typically issue the copy line by line, when copy the last
>> line, out-of-bound access will be happen. Because:
>>
>>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>>
>> Note that userspace may stil write to the invisiable area if a larger
>> buffer than width x stride is exposed. But it is not a big issue as
>> long as there still have memory resolve the access if not drafting so
>> far.
>>
>>   - Also limit the y1 (Daniel)
>>   - keep fix patch it to minimal (Daniel)
>>   - screen_size is page size aligned because of it need mmap (Thomas)
>>   - Adding fixes tag (Thomas)
>>
>> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Thanks for the update!  This v5 is completely different from the v3
> I tested before, so keeping my Tested-by is not really appropriate...
Indeed, I will be carefully next time.
> I have retested fbtest with shmob-drm on Armadillo-800-EVA
> (800x480@RG16, i.e. 187.5 pages), and fortunately this version still
> works fine, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Thanks a lot.
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
Sui Jingfeng April 20, 2023, 10:04 a.m. UTC | #5 
Hi

On 2023/4/20 15:07, Thomas Zimmermann wrote:
> Hi
>
> Am 20.04.23 um 09:04 schrieb Thomas Zimmermann:
>> Hi,
>>
>> this patch looks to me good and I'd like to merge it, if no one objects.
>
> Rereading it, I might have been too eager. What happened to the 
> setting of screen_size = buffer->gem->size ?  It is not relevant?
>
Short answer is that it is not relevant.

As long as the computed damage rectangle is sane, it's OK to allocate a 
bit more than needed.

I think it's turn out to be *correct*, if not extremely.

Because it is page size aligned,  writing to invisible area for some 
case is not a serve issue.

It also guarantee that the size of shadow screen buffer is exactly the 
same size with its GEM counterpart.


Otherwise I have to answer the question

What will happen if the 'screen_size' is not page_size aligned and mmap 
will mapping in the granularity of pages ?


I see efifb also align the buffer going to be mapped with page size.


> Best regards
> Thomas
>
>>
>> In the near future, after i915 has switched to struct drm_client, I 
>> intend to move DRM's deferred-I/O helpers into fbdev-generic and 
>> i915. Those are the two users, but they are fairly different. They 
>> can then both have something tailored towards their needs.
>>
>> Best regards
>> Thomas
>>
>> Am 20.04.23 um 05:05 schrieb Sui Jingfeng:
>>> The fbdev test of IGT may write after EOF, which lead to out-of-bound
>>> access for drm drivers hire fbdev-generic. For example, run fbdev test
>>> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
>>> linux kernel 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 ]---
>>>
>>> The is because damage rectangles computed by
>>> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
>>> bound in the screen's active display area. Possible reasons are:
>>>
>>> 1) Buffers are allocated in the granularity of page size, for mmap 
>>> system
>>>     call support. The shadow screen buffer consumed by fbdev 
>>> emulation may
>>>     also choosed be page size aligned.
>>>
>>> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>>>     will introduce off-by-one error.
>>>
>>> For example, on a 16KB page size system, in order to store a 1920x1080
>>> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
>>> 1920*1080*4 can not be divided exactly by 16KB.
>>>
>>>   1920 * 1080 * 4 = 8294400 bytes
>>>   506 * 16 * 1024 = 8290304 bytes
>>>   507 * 16 * 1024 = 8306688 bytes
>>>
>>>   line_length = 1920*4 = 7680 bytes
>>>
>>>   507 * 16 * 1024 / 7680 = 1081.6
>>>
>>>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>>>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>>>
>>> memcpy_toio() typically issue the copy line by line, when copy the last
>>> line, out-of-bound access will be happen. Because:
>>>
>>>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>>>
>>> Note that userspace may stil write to the invisiable area if a larger
>>> buffer than width x stride is exposed. But it is not a big issue as
>>> long as there still have memory resolve the access if not drafting so
>>> far.
>>>
>>>   - Also limit the y1 (Daniel)
>>>   - keep fix patch it to minimal (Daniel)
>>>   - screen_size is page size aligned because of it need mmap (Thomas)
>>>   - Adding fixes tag (Thomas)
>>>
>>> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>>>
>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Link: 
>>> https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
>>> ---
>>>   drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 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;
>>>           x1 = bit_off / info->var.bits_per_pixel;
>>>           x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
>>
>
Sui Jingfeng April 20, 2023, 10:09 a.m. UTC | #6 
Hi,

On 2023/4/20 17:04, Geert Uytterhoeven wrote:
> Hi Sui,
>
> On Thu, Apr 20, 2023 at 5:09 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>> The fbdev test of IGT may write after EOF, which lead to out-of-bound
>> access for drm drivers hire fbdev-generic. For example, run fbdev test
>> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
>> linux kernel 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 ]---
>>
>> The is because damage rectangles computed by
>> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
>> bound in the screen's active display area. Possible reasons are:
>>
>> 1) Buffers are allocated in the granularity of page size, for mmap system
>>     call support. The shadow screen buffer consumed by fbdev emulation may
>>     also choosed be page size aligned.
>>
>> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>>     will introduce off-by-one error.
>>
>> For example, on a 16KB page size system, in order to store a 1920x1080
>> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
>> 1920*1080*4 can not be divided exactly by 16KB.
>>
>>   1920 * 1080 * 4 = 8294400 bytes
>>   506 * 16 * 1024 = 8290304 bytes
>>   507 * 16 * 1024 = 8306688 bytes
>>
>>   line_length = 1920*4 = 7680 bytes
>>
>>   507 * 16 * 1024 / 7680 = 1081.6
>>
>>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>>
>> memcpy_toio() typically issue the copy line by line, when copy the last
>> line, out-of-bound access will be happen. Because:
>>
>>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>>
>> Note that userspace may stil write to the invisiable area if a larger
>> buffer than width x stride is exposed. But it is not a big issue as
>> long as there still have memory resolve the access if not drafting so
>> far.
>>
>>   - Also limit the y1 (Daniel)
>>   - keep fix patch it to minimal (Daniel)
>>   - screen_size is page size aligned because of it need mmap (Thomas)
>>   - Adding fixes tag (Thomas)
>>
>> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Thanks for the update!  This v5 is completely different from the v3
> I tested before, so keeping my Tested-by is not really appropriate...

Thanks for testing. I'm a bit of confident that it will works.

Your tested-by is valuable, really don't want drop this. So I keep it.

> I have retested fbtest with shmob-drm on Armadillo-800-EVA
> (800x480@RG16, i.e. 187.5 pages), and fortunately this version still
> works fine, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
Sui Jingfeng April 20, 2023, 10:26 a.m. UTC | #7 
Hi,

On 2023/4/20 15:04, Thomas Zimmermann wrote:
> Hi,
>
> this patch looks to me good and I'd like to merge it, if no one objects.
>
> In the near future, after i915 has switched to struct drm_client, I 
> intend to move DRM's deferred-I/O helpers into fbdev-generic and i915. 


That sound fine,  I can help to test then.

But I think,  it may better to make it a 100% DRM function?


> Those are the two users, but they are fairly different. They can then 
> both have something tailored towards their needs.
>
> Best regards
> Thomas
>
> Am 20.04.23 um 05:05 schrieb Sui Jingfeng:
>> The fbdev test of IGT may write after EOF, which lead to out-of-bound
>> access for drm drivers hire fbdev-generic. For example, run fbdev test
>> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
>> linux kernel 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 ]---
>>
>> The is because damage rectangles computed by
>> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
>> bound in the screen's active display area. Possible reasons are:
>>
>> 1) Buffers are allocated in the granularity of page size, for mmap 
>> system
>>     call support. The shadow screen buffer consumed by fbdev 
>> emulation may
>>     also choosed be page size aligned.
>>
>> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>>     will introduce off-by-one error.
>>
>> For example, on a 16KB page size system, in order to store a 1920x1080
>> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
>> 1920*1080*4 can not be divided exactly by 16KB.
>>
>>   1920 * 1080 * 4 = 8294400 bytes
>>   506 * 16 * 1024 = 8290304 bytes
>>   507 * 16 * 1024 = 8306688 bytes
>>
>>   line_length = 1920*4 = 7680 bytes
>>
>>   507 * 16 * 1024 / 7680 = 1081.6
>>
>>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>>
>> memcpy_toio() typically issue the copy line by line, when copy the last
>> line, out-of-bound access will be happen. Because:
>>
>>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>>
>> Note that userspace may stil write to the invisiable area if a larger
>> buffer than width x stride is exposed. But it is not a big issue as
>> long as there still have memory resolve the access if not drafting so
>> far.
>>
>>   - Also limit the y1 (Daniel)
>>   - keep fix patch it to minimal (Daniel)
>>   - screen_size is page size aligned because of it need mmap (Thomas)
>>   - Adding fixes tag (Thomas)
>>
>> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Link: 
>> https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 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;
>>             x1 = bit_off / info->var.bits_per_pixel;
>>           x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
>
Thomas Zimmermann April 20, 2023, 11:10 a.m. UTC | #8 
Hi

Am 20.04.23 um 12:04 schrieb Sui Jingfeng:
> Hi
> 
> On 2023/4/20 15:07, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 20.04.23 um 09:04 schrieb Thomas Zimmermann:
>>> Hi,
>>>
>>> this patch looks to me good and I'd like to merge it, if no one objects.
>>
>> Rereading it, I might have been too eager. What happened to the 
>> setting of screen_size = buffer->gem->size ?  It is not relevant?
>>
> Short answer is that it is not relevant.
> 
> As long as the computed damage rectangle is sane, it's OK to allocate a 
> bit more than needed.
> 
> I think it's turn out to be *correct*, if not extremely.
> 
> Because it is page size aligned,  writing to invisible area for some 
> case is not a serve issue.
> 
> It also guarantee that the size of shadow screen buffer is exactly the 
> same size with its GEM counterpart.

That's good enough for me. :)

> 
> 
> Otherwise I have to answer the question
> 
> What will happen if the 'screen_size' is not page_size aligned and mmap 
> will mapping in the granularity of pages ?

You need to map at page granularity. If screen_size is not page-size 
aligned, there's this trailing buffer that is accessible, but cannot be 
displayed. But userspace has no direct way of knowing that, so let's 
ignore that problem for now.

Best regards
Thomas

> 
> 
> I see efifb also align the buffer going to be mapped with page size.
> 
> 
>> Best regards
>> Thomas
>>
>>>
>>> In the near future, after i915 has switched to struct drm_client, I 
>>> intend to move DRM's deferred-I/O helpers into fbdev-generic and 
>>> i915. Those are the two users, but they are fairly different. They 
>>> can then both have something tailored towards their needs.
>>>
>>> Best regards
>>> Thomas
>>>
>>> Am 20.04.23 um 05:05 schrieb Sui Jingfeng:
>>>> The fbdev test of IGT may write after EOF, which lead to out-of-bound
>>>> access for drm drivers hire fbdev-generic. For example, run fbdev test
>>>> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
>>>> linux kernel 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 ]---
>>>>
>>>> The is because damage rectangles computed by
>>>> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
>>>> bound in the screen's active display area. Possible reasons are:
>>>>
>>>> 1) Buffers are allocated in the granularity of page size, for mmap 
>>>> system
>>>>     call support. The shadow screen buffer consumed by fbdev 
>>>> emulation may
>>>>     also choosed be page size aligned.
>>>>
>>>> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>>>>     will introduce off-by-one error.
>>>>
>>>> For example, on a 16KB page size system, in order to store a 1920x1080
>>>> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
>>>> 1920*1080*4 can not be divided exactly by 16KB.
>>>>
>>>>   1920 * 1080 * 4 = 8294400 bytes
>>>>   506 * 16 * 1024 = 8290304 bytes
>>>>   507 * 16 * 1024 = 8306688 bytes
>>>>
>>>>   line_length = 1920*4 = 7680 bytes
>>>>
>>>>   507 * 16 * 1024 / 7680 = 1081.6
>>>>
>>>>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>>>>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>>>>
>>>> memcpy_toio() typically issue the copy line by line, when copy the last
>>>> line, out-of-bound access will be happen. Because:
>>>>
>>>>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>>>>
>>>> Note that userspace may stil write to the invisiable area if a larger
>>>> buffer than width x stride is exposed. But it is not a big issue as
>>>> long as there still have memory resolve the access if not drafting so
>>>> far.
>>>>
>>>>   - Also limit the y1 (Daniel)
>>>>   - keep fix patch it to minimal (Daniel)
>>>>   - screen_size is page size aligned because of it need mmap (Thomas)
>>>>   - Adding fixes tag (Thomas)
>>>>
>>>> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Link: 
>>>> https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
>>>> ---
>>>>   drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
>>>>   1 file changed, 12 insertions(+), 4 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;
>>>>           x1 = bit_off / info->var.bits_per_pixel;
>>>>           x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
>>>
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Geert Uytterhoeven April 20, 2023, 12:25 p.m. UTC | #9 
Hi Thomas,

On Thu, Apr 20, 2023 at 1:10 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 20.04.23 um 12:04 schrieb Sui Jingfeng:
> > What will happen if the 'screen_size' is not page_size aligned and mmap
> > will mapping in the granularity of pages ?
>
> You need to map at page granularity. If screen_size is not page-size
> aligned, there's this trailing buffer that is accessible, but cannot be
> displayed. But userspace has no direct way of knowing that, so let's
> ignore that problem for now.

Userspace can know, if fb_fix_screeninfo.smem_{start,len} match
the actual offset and size.

Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann April 20, 2023, 12:59 p.m. UTC | #10 
Hi

Am 20.04.23 um 14:25 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Thu, Apr 20, 2023 at 1:10 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 20.04.23 um 12:04 schrieb Sui Jingfeng:
>>> What will happen if the 'screen_size' is not page_size aligned and mmap
>>> will mapping in the granularity of pages ?
>>
>> You need to map at page granularity. If screen_size is not page-size
>> aligned, there's this trailing buffer that is accessible, but cannot be
>> displayed. But userspace has no direct way of knowing that, so let's
>> ignore that problem for now.
> 
> Userspace can know, if fb_fix_screeninfo.smem_{start,len} match
> the actual offset and size.

Can you elaborate? How can userspace detect/compute the actually usable 
space?

 From grep'ing fbdev drivers, smem_len appears to be a multiple of the 
pagesize. (?) screen_size is not exported and line_length in the fixed 
portion. Or can line_length change between modes? In that case it should 
be (yres_virtual * line_length), right?

Best regards
Thomas


> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Geert Uytterhoeven April 20, 2023, 2:21 p.m. UTC | #11 
Hi Thomas,

On Thu, Apr 20, 2023 at 2:59 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 20.04.23 um 14:25 schrieb Geert Uytterhoeven:
> > On Thu, Apr 20, 2023 at 1:10 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Am 20.04.23 um 12:04 schrieb Sui Jingfeng:
> >>> What will happen if the 'screen_size' is not page_size aligned and mmap
> >>> will mapping in the granularity of pages ?
> >>
> >> You need to map at page granularity. If screen_size is not page-size
> >> aligned, there's this trailing buffer that is accessible, but cannot be
> >> displayed. But userspace has no direct way of knowing that, so let's
> >> ignore that problem for now.
> >
> > Userspace can know, if fb_fix_screeninfo.smem_{start,len} match
> > the actual offset and size.
>
> Can you elaborate? How can userspace detect/compute the actually usable
> space?

By looking at fb_fix_screeninfo.smem_{start,len}, which are the (physical)
address and length of the frame buffer.

>  From grep'ing fbdev drivers, smem_len appears to be a multiple of the
> pagesize. (?) screen_size is not exported and line_length in the fixed
>
> portion. Or can line_length change between modes? In that case it should
> be (yres_virtual * line_length), right?

smem_{start,len} are not guaranteed to be page-aligned.
Fbtest[1] and Xorg should handle that fine.

line_length can change between modes.  It's part of fb_fix_screeninfo,
because it's fixed in the sense that it depends on the video mode, and
the user cannot specify its value.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git/tree/fb.c#n199

Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann April 21, 2023, 8:09 a.m. UTC | #12 
Hi

Am 20.04.23 um 05:05 schrieb Sui Jingfeng:
> The fbdev test of IGT may write after EOF, which lead to out-of-bound
> access for drm drivers hire fbdev-generic. For example, run fbdev test
> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
> linux kernel hang with the following call trace:

I've merged this patch into drm-misc-fixes. Thanks for the fix.

Best regards
Thomas

> 
>    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 is because damage rectangles computed by
> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
> bound in the screen's active display area. Possible reasons are:
> 
> 1) Buffers are allocated in the granularity of page size, for mmap system
>     call support. The shadow screen buffer consumed by fbdev emulation may
>     also choosed be page size aligned.
> 
> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>     will introduce off-by-one error.
> 
> For example, on a 16KB page size system, in order to store a 1920x1080
> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
> 1920*1080*4 can not be divided exactly by 16KB.
> 
>   1920 * 1080 * 4 = 8294400 bytes
>   506 * 16 * 1024 = 8290304 bytes
>   507 * 16 * 1024 = 8306688 bytes
> 
>   line_length = 1920*4 = 7680 bytes
> 
>   507 * 16 * 1024 / 7680 = 1081.6
> 
>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
> 
> memcpy_toio() typically issue the copy line by line, when copy the last
> line, out-of-bound access will be happen. Because:
> 
>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
> 
> Note that userspace may stil write to the invisiable area if a larger
> buffer than width x stride is exposed. But it is not a big issue as
> long as there still have memory resolve the access if not drafting so
> far.
> 
>   - Also limit the y1 (Daniel)
>   - keep fix patch it to minimal (Daniel)
>   - screen_size is page size aligned because of it need mmap (Thomas)
>   - Adding fixes tag (Thomas)
> 
> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Link: https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 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;
>   
>   		x1 = bit_off / info->var.bits_per_pixel;
>   		x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Sui Jingfeng April 21, 2023, 8:24 a.m. UTC | #13 
Hi

On 2023/4/21 16:09, Thomas Zimmermann wrote:
> Hi
>
> Am 20.04.23 um 05:05 schrieb Sui Jingfeng:
>> The fbdev test of IGT may write after EOF, which lead to out-of-bound
>> access for drm drivers hire fbdev-generic. For example, run fbdev test
>> on a x86+ast2400 platform, with 1680x1050 resolution, will cause the
>> linux kernel hang with the following call trace:
>
> I've merged this patch into drm-misc-fixes. Thanks for the fix.
>

Thanks a lot!


> Best regards
> Thomas
>
>>
>>    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 is because damage rectangles computed by
>> drm_fb_helper_memory_range_to_clip() function does not guaranteed to be
>> bound in the screen's active display area. Possible reasons are:
>>
>> 1) Buffers are allocated in the granularity of page size, for mmap 
>> system
>>     call support. The shadow screen buffer consumed by fbdev 
>> emulation may
>>     also choosed be page size aligned.
>>
>> 2) The DIV_ROUND_UP() used in drm_fb_helper_memory_range_to_clip()
>>     will introduce off-by-one error.
>>
>> For example, on a 16KB page size system, in order to store a 1920x1080
>> XRGB framebuffer, we need allocate 507 pages. Unfortunately, the size
>> 1920*1080*4 can not be divided exactly by 16KB.
>>
>>   1920 * 1080 * 4 = 8294400 bytes
>>   506 * 16 * 1024 = 8290304 bytes
>>   507 * 16 * 1024 = 8306688 bytes
>>
>>   line_length = 1920*4 = 7680 bytes
>>
>>   507 * 16 * 1024 / 7680 = 1081.6
>>
>>   off / line_length = 507 * 16 * 1024 / 7680 = 1081
>>   DIV_ROUND_UP(507 * 16 * 1024, 7680) will yeild 1082
>>
>> memcpy_toio() typically issue the copy line by line, when copy the last
>> line, out-of-bound access will be happen. Because:
>>
>>   1082 * line_length = 1082 * 7680 = 8309760, and 8309760 > 8306688
>>
>> Note that userspace may stil write to the invisiable area if a larger
>> buffer than width x stride is exposed. But it is not a big issue as
>> long as there still have memory resolve the access if not drafting so
>> far.
>>
>>   - Also limit the y1 (Daniel)
>>   - keep fix patch it to minimal (Daniel)
>>   - screen_size is page size aligned because of it need mmap (Thomas)
>>   - Adding fixes tag (Thomas)
>>
>> Fixes: aa15c677cc34 ("drm/fb-helper: Fix vertical damage clipping")
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Link: 
>> https://lore.kernel.org/dri-devel/ad44df29-3241-0d9e-e708-b0338bf3c623@189.cn/
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 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;
>>             x1 = bit_off / info->var.bits_per_pixel;
>>           x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);
>
diff mbox series

Patch

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;
 
 		x1 = bit_off / info->var.bits_per_pixel;
 		x2 = DIV_ROUND_UP(bit_end, info->var.bits_per_pixel);