* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
@ 2023-04-17 7:29 ` Thomas Zimmermann
0 siblings, 0 replies; 20+ messages in thread
From: Thomas Zimmermann @ 2023-04-17 7:29 UTC (permalink / raw)
To: Sui Jingfeng, Maxime Ripard, David Airlie, Daniel Vetter,
Sui Jingfeng, Li Yi, Javier Martinez Canillas, Helge Deller,
Lucas De Marchi
Cc: linux-kernel, linux-fbdev, dri-devel, loongson-kernel
[-- Attachment #1.1: Type: text/plain, Size: 6864 bytes --]
Hi
Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
> Hi,
>
> On 2023/4/14 03:16, Thomas Zimmermann wrote:
>> 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@loongson.cn>
>>>
>>> The crazy fbdev test of IGT may write after EOF, which lead to
>>> out-of-bound
>>
>> Please drop 'crazy'. :)
>
> This is OK.
>
> By using the world 'crazy',
>
> I meant that the test is very good and maybe it is written by
> professional peoples
>
> with the guidance by experienced engineer. So that even the corner get
> tested.
>
>
>>
>>> 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@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.
>
>
> Are you meant that we should remove all these changes in
> drivers/gpu/drm/drm_fb_helper.c ?
As Daniel mentioned, there's the discussion in the other thread. I don't
want to reopen it here. Just to summarize: I'm not convinced that this
should be DRM code because it can be shared with other fbdev drivers.
[...]
>>> 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];
This has been bothering me over the weekend. And I think it's because
what we want the screen_size to be heigth * pitch, but the mmap'ed
memory is still at page granularity. Therefore...
[...]
>>
>>> screen_buffer = vzalloc(screen_size);
... this line should explicitly allocate multiples of pages. Something like
/* allocate page-size multiples for mmap */
vzalloc(PAGE_ALIGN(screen_size))
It has not been a bug so far because vzalloc() always returns full pages
IIRC. It's still worth fixing.
Best regards
Thomas
>>> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
2023-04-17 7:29 ` Thomas Zimmermann
@ 2023-04-17 8:08 ` Sui Jingfeng
-1 siblings, 0 replies; 20+ messages in thread
From: Sui Jingfeng @ 2023-04-17 8:08 UTC (permalink / raw)
To: Thomas Zimmermann, Maxime Ripard, David Airlie, Daniel Vetter,
Sui Jingfeng, Li Yi, Javier Martinez Canillas, Helge Deller,
Lucas De Marchi
Cc: linux-kernel, linux-fbdev, dri-devel, loongson-kernel
Hi,
On 2023/4/17 15:29, Thomas Zimmermann wrote:
> Hi
>
> Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/4/14 03:16, Thomas Zimmermann wrote:
>>> 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@loongson.cn>
>>>>
>>>> The crazy fbdev test of IGT may write after EOF, which lead to
>>>> out-of-bound
>>>
>>> Please drop 'crazy'. :)
>>
>> This is OK.
>>
>> By using the world 'crazy',
>>
>> I meant that the test is very good and maybe it is written by
>> professional peoples
>>
>> with the guidance by experienced engineer. So that even the corner
>> get tested.
>>
>>
>>>
>>>> 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@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.
>>
>>
>> Are you meant that we should remove all these changes in
>> drivers/gpu/drm/drm_fb_helper.c ?
>
> As Daniel mentioned, there's the discussion in the other thread. I
> don't want to reopen it here. Just to summarize: I'm not convinced
> that this should be DRM code because it can be shared with other fbdev
> drivers.
>
> [...]
>
>>>> 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];
>
> This has been bothering me over the weekend. And I think it's because
> what we want the screen_size to be heigth * pitch, but the mmap'ed
> memory is still at page granularity. Therefore...
>
Yeah, this bug is not that simple as it seems, I will drop the
controversy part, leave it there, it may need more time to rethink about
it.
Thanks for reviewing, don't be so tired...
> [...]
>>>
>>>> screen_buffer = vzalloc(screen_size);
>
> ... this line should explicitly allocate multiples of pages. Something
> like
>
> /* allocate page-size multiples for mmap */
> vzalloc(PAGE_ALIGN(screen_size))
>
> It has not been a bug so far because vzalloc() always returns full
> pages IIRC. It's still worth fixing.
>
> Best regards
> Thomas
>
>
>>>> if (!screen_buffer) {
>>>> ret = -ENOMEM;
>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
@ 2023-04-17 8:08 ` Sui Jingfeng
0 siblings, 0 replies; 20+ messages in thread
From: Sui Jingfeng @ 2023-04-17 8:08 UTC (permalink / raw)
To: Thomas Zimmermann, Maxime Ripard, David Airlie, Daniel Vetter,
Sui Jingfeng, Li Yi, Javier Martinez Canillas, Helge Deller,
Lucas De Marchi
Cc: loongson-kernel, linux-fbdev, linux-kernel, dri-devel
Hi,
On 2023/4/17 15:29, Thomas Zimmermann wrote:
> Hi
>
> Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/4/14 03:16, Thomas Zimmermann wrote:
>>> 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@loongson.cn>
>>>>
>>>> The crazy fbdev test of IGT may write after EOF, which lead to
>>>> out-of-bound
>>>
>>> Please drop 'crazy'. :)
>>
>> This is OK.
>>
>> By using the world 'crazy',
>>
>> I meant that the test is very good and maybe it is written by
>> professional peoples
>>
>> with the guidance by experienced engineer. So that even the corner
>> get tested.
>>
>>
>>>
>>>> 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@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.
>>
>>
>> Are you meant that we should remove all these changes in
>> drivers/gpu/drm/drm_fb_helper.c ?
>
> As Daniel mentioned, there's the discussion in the other thread. I
> don't want to reopen it here. Just to summarize: I'm not convinced
> that this should be DRM code because it can be shared with other fbdev
> drivers.
>
> [...]
>
>>>> 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];
>
> This has been bothering me over the weekend. And I think it's because
> what we want the screen_size to be heigth * pitch, but the mmap'ed
> memory is still at page granularity. Therefore...
>
Yeah, this bug is not that simple as it seems, I will drop the
controversy part, leave it there, it may need more time to rethink about
it.
Thanks for reviewing, don't be so tired...
> [...]
>>>
>>>> screen_buffer = vzalloc(screen_size);
>
> ... this line should explicitly allocate multiples of pages. Something
> like
>
> /* allocate page-size multiples for mmap */
> vzalloc(PAGE_ALIGN(screen_size))
>
> It has not been a bug so far because vzalloc() always returns full
> pages IIRC. It's still worth fixing.
>
> Best regards
> Thomas
>
>
>>>> if (!screen_buffer) {
>>>> ret = -ENOMEM;
>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
2023-04-17 7:29 ` Thomas Zimmermann
@ 2023-04-17 8:42 ` Daniel Vetter
-1 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2023-04-17 8:42 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: linux-fbdev, Sui Jingfeng, Li Yi, Helge Deller, Lucas De Marchi,
Javier Martinez Canillas, Sui Jingfeng, loongson-kernel,
dri-devel, linux-kernel
On Mon, Apr 17, 2023 at 09:29:07AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
> > Hi,
> >
> > On 2023/4/14 03:16, Thomas Zimmermann wrote:
> > > 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@loongson.cn>
> > > >
> > > > The crazy fbdev test of IGT may write after EOF, which lead to
> > > > out-of-bound
> > >
> > > Please drop 'crazy'. :)
> >
> > This is OK.
> >
> > By using the world 'crazy',
> >
> > I meant that the test is very good and maybe it is written by
> > professional peoples
> >
> > with the guidance by experienced engineer. So that even the corner get
> > tested.
> >
> >
> > >
> > > > 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@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.
> >
> >
> > Are you meant that we should remove all these changes in
> > drivers/gpu/drm/drm_fb_helper.c ?
>
> As Daniel mentioned, there's the discussion in the other thread. I don't
> want to reopen it here. Just to summarize: I'm not convinced that this
> should be DRM code because it can be shared with other fbdev drivers.
>
> [...]
>
> > > > 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];
>
> This has been bothering me over the weekend. And I think it's because what
> we want the screen_size to be heigth * pitch, but the mmap'ed memory is
> still at page granularity. Therefore...
>
> [...]
> > >
> > > > screen_buffer = vzalloc(screen_size);
>
> ... this line should explicitly allocate multiples of pages. Something like
>
> /* allocate page-size multiples for mmap */
> vzalloc(PAGE_ALIGN(screen_size))
>
> It has not been a bug so far because vzalloc() always returns full pages
> IIRC. It's still worth fixing.
So gem_bo are supposed to be at least PAGE_SIZE aligned. So maybe we just
put a comment in here why this assumption holds, and reinforce it with a
WARN_ON? I think that would address the concerns here?
But I concur with Sui that this is a separate issue which should be sorted
out in a separate patch.
-Daniel
>
> Best regards
> Thomas
>
>
> > > > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
@ 2023-04-17 8:42 ` Daniel Vetter
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2023-04-17 8:42 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Sui Jingfeng, Maxime Ripard, David Airlie, Daniel Vetter,
Sui Jingfeng, Li Yi, Javier Martinez Canillas, Helge Deller,
Lucas De Marchi, linux-kernel, linux-fbdev, dri-devel,
loongson-kernel
On Mon, Apr 17, 2023 at 09:29:07AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
> > Hi,
> >
> > On 2023/4/14 03:16, Thomas Zimmermann wrote:
> > > 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@loongson.cn>
> > > >
> > > > The crazy fbdev test of IGT may write after EOF, which lead to
> > > > out-of-bound
> > >
> > > Please drop 'crazy'. :)
> >
> > This is OK.
> >
> > By using the world 'crazy',
> >
> > I meant that the test is very good and maybe it is written by
> > professional peoples
> >
> > with the guidance by experienced engineer. So that even the corner get
> > tested.
> >
> >
> > >
> > > > 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@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.
> >
> >
> > Are you meant that we should remove all these changes in
> > drivers/gpu/drm/drm_fb_helper.c ?
>
> As Daniel mentioned, there's the discussion in the other thread. I don't
> want to reopen it here. Just to summarize: I'm not convinced that this
> should be DRM code because it can be shared with other fbdev drivers.
>
> [...]
>
> > > > 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];
>
> This has been bothering me over the weekend. And I think it's because what
> we want the screen_size to be heigth * pitch, but the mmap'ed memory is
> still at page granularity. Therefore...
>
> [...]
> > >
> > > > screen_buffer = vzalloc(screen_size);
>
> ... this line should explicitly allocate multiples of pages. Something like
>
> /* allocate page-size multiples for mmap */
> vzalloc(PAGE_ALIGN(screen_size))
>
> It has not been a bug so far because vzalloc() always returns full pages
> IIRC. It's still worth fixing.
So gem_bo are supposed to be at least PAGE_SIZE aligned. So maybe we just
put a comment in here why this assumption holds, and reinforce it with a
WARN_ON? I think that would address the concerns here?
But I concur with Sui that this is a separate issue which should be sorted
out in a separate patch.
-Daniel
>
> Best regards
> Thomas
>
>
> > > > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
2023-04-17 7:29 ` Thomas Zimmermann
@ 2023-04-19 18:47 ` Sui Jingfeng
-1 siblings, 0 replies; 20+ messages in thread
From: Sui Jingfeng @ 2023-04-19 18:47 UTC (permalink / raw)
To: Thomas Zimmermann, Maxime Ripard, David Airlie, Daniel Vetter,
Sui Jingfeng, Li Yi, Javier Martinez Canillas, Helge Deller,
Lucas De Marchi
Cc: linux-kernel, linux-fbdev, dri-devel, loongson-kernel
Hi,
On 2023/4/17 15:29, Thomas Zimmermann wrote:
> Hi
>
> Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/4/14 03:16, Thomas Zimmermann wrote:
>>> 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@loongson.cn>
>>>>
>>>> The crazy fbdev test of IGT may write after EOF, which lead to
>>>> out-of-bound
>>>
>>> Please drop 'crazy'. :)
>>
>> This is OK.
>>
>> By using the world 'crazy',
>>
>> I meant that the test is very good and maybe it is written by
>> professional peoples
>>
>> with the guidance by experienced engineer. So that even the corner
>> get tested.
>>
>>
>>>
>>>> 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@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.
>>
>>
>> Are you meant that we should remove all these changes in
>> drivers/gpu/drm/drm_fb_helper.c ?
>
> As Daniel mentioned, there's the discussion in the other thread. I
> don't want to reopen it here. Just to summarize: I'm not convinced
> that this should be DRM code because it can be shared with other fbdev
> drivers.
>
> [...]
>
>>>> 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];
>
> This has been bothering me over the weekend. And I think it's because
> what we want the screen_size to be heigth * pitch, but the mmap'ed
> memory is still at page granularity. Therefore...
>
> [...]
>>>
>>>> screen_buffer = vzalloc(screen_size);
>
> ... this line should explicitly allocate multiples of pages. Something
> like
>
> /* allocate page-size multiples for mmap */
> vzalloc(PAGE_ALIGN(screen_size))
>
I just thought about your instruction at here, thanks!
But it is already page size aligned if we don't tough it.
It is guaranteed by the GEM memory manger,
so a previous patch tested by me, is turn out to be a extremely correct?
We exposed a page size aligned buffer(even though it is larger than
needed) is actually for mmap ?
> It has not been a bug so far because vzalloc() always returns full
> pages IIRC. It's still worth fixing.
>
> Best regards
> Thomas
>
>
>>>> if (!screen_buffer) {
>>>> ret = -ENOMEM;
>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
@ 2023-04-19 18:47 ` Sui Jingfeng
0 siblings, 0 replies; 20+ messages in thread
From: Sui Jingfeng @ 2023-04-19 18:47 UTC (permalink / raw)
To: Thomas Zimmermann, Maxime Ripard, David Airlie, Daniel Vetter,
Sui Jingfeng, Li Yi, Javier Martinez Canillas, Helge Deller,
Lucas De Marchi
Cc: loongson-kernel, linux-fbdev, linux-kernel, dri-devel
Hi,
On 2023/4/17 15:29, Thomas Zimmermann wrote:
> Hi
>
> Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/4/14 03:16, Thomas Zimmermann wrote:
>>> 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@loongson.cn>
>>>>
>>>> The crazy fbdev test of IGT may write after EOF, which lead to
>>>> out-of-bound
>>>
>>> Please drop 'crazy'. :)
>>
>> This is OK.
>>
>> By using the world 'crazy',
>>
>> I meant that the test is very good and maybe it is written by
>> professional peoples
>>
>> with the guidance by experienced engineer. So that even the corner
>> get tested.
>>
>>
>>>
>>>> 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@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.
>>
>>
>> Are you meant that we should remove all these changes in
>> drivers/gpu/drm/drm_fb_helper.c ?
>
> As Daniel mentioned, there's the discussion in the other thread. I
> don't want to reopen it here. Just to summarize: I'm not convinced
> that this should be DRM code because it can be shared with other fbdev
> drivers.
>
> [...]
>
>>>> 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];
>
> This has been bothering me over the weekend. And I think it's because
> what we want the screen_size to be heigth * pitch, but the mmap'ed
> memory is still at page granularity. Therefore...
>
> [...]
>>>
>>>> screen_buffer = vzalloc(screen_size);
>
> ... this line should explicitly allocate multiples of pages. Something
> like
>
> /* allocate page-size multiples for mmap */
> vzalloc(PAGE_ALIGN(screen_size))
>
I just thought about your instruction at here, thanks!
But it is already page size aligned if we don't tough it.
It is guaranteed by the GEM memory manger,
so a previous patch tested by me, is turn out to be a extremely correct?
We exposed a page size aligned buffer(even though it is larger than
needed) is actually for mmap ?
> It has not been a bug so far because vzalloc() always returns full
> pages IIRC. It's still worth fixing.
>
> Best regards
> Thomas
>
>
>>>> if (!screen_buffer) {
>>>> ret = -ENOMEM;
>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
2023-04-19 18:47 ` Sui Jingfeng
@ 2023-04-27 9:34 ` Daniel Vetter
-1 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2023-04-27 9:34 UTC (permalink / raw)
To: Sui Jingfeng
Cc: Thomas Zimmermann, Maxime Ripard, David Airlie, Daniel Vetter,
Sui Jingfeng, Li Yi, Javier Martinez Canillas, Helge Deller,
Lucas De Marchi, linux-kernel, linux-fbdev, dri-devel,
loongson-kernel
On Thu, Apr 20, 2023 at 02:47:24AM +0800, Sui Jingfeng wrote:
> Hi,
>
> On 2023/4/17 15:29, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
> > > Hi,
> > >
> > > On 2023/4/14 03:16, Thomas Zimmermann wrote:
> > > > 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@loongson.cn>
> > > > >
> > > > > The crazy fbdev test of IGT may write after EOF, which lead
> > > > > to out-of-bound
> > > >
> > > > Please drop 'crazy'. :)
> > >
> > > This is OK.
> > >
> > > By using the world 'crazy',
> > >
> > > I meant that the test is very good and maybe it is written by
> > > professional peoples
> > >
> > > with the guidance by experienced engineer. So that even the corner
> > > get tested.
> > >
> > >
> > > >
> > > > > 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@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.
> > >
> > >
> > > Are you meant that we should remove all these changes in
> > > drivers/gpu/drm/drm_fb_helper.c ?
> >
> > As Daniel mentioned, there's the discussion in the other thread. I don't
> > want to reopen it here. Just to summarize: I'm not convinced that this
> > should be DRM code because it can be shared with other fbdev drivers.
> >
> > [...]
> >
> > > > > 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];
> >
> > This has been bothering me over the weekend. And I think it's because
> > what we want the screen_size to be heigth * pitch, but the mmap'ed
> > memory is still at page granularity. Therefore...
> >
> > [...]
> > > >
> > > > > screen_buffer = vzalloc(screen_size);
> >
> > ... this line should explicitly allocate multiples of pages. Something
> > like
> >
> > /* allocate page-size multiples for mmap */
> > vzalloc(PAGE_ALIGN(screen_size))
> >
> I just thought about your instruction at here, thanks!
>
> But it is already page size aligned if we don't tough it.
>
> It is guaranteed by the GEM memory manger,
>
> so a previous patch tested by me, is turn out to be a extremely correct?
>
> We exposed a page size aligned buffer(even though it is larger than needed)
> is actually for mmap ?
mmap() is always page aligned, because that's the smallest size the cpu
pagetables can map. So there's fundamentally always a bit of memory at the
end of the buffer which logically is not part of the framebuffer memory.
And somehow we need to handle that to make sure we don't overflow.
-Daniel
>
>
> > It has not been a bug so far because vzalloc() always returns full pages
> > IIRC. It's still worth fixing.
> >
>
> > Best regards
> > Thomas
> >
> >
> > > > > if (!screen_buffer) {
> > > > > ret = -ENOMEM;
> > > >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
@ 2023-04-27 9:34 ` Daniel Vetter
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2023-04-27 9:34 UTC (permalink / raw)
To: Sui Jingfeng
Cc: linux-fbdev, Sui Jingfeng, Thomas Zimmermann, Li Yi,
Helge Deller, Lucas De Marchi, Javier Martinez Canillas,
linux-kernel, loongson-kernel, dri-devel
On Thu, Apr 20, 2023 at 02:47:24AM +0800, Sui Jingfeng wrote:
> Hi,
>
> On 2023/4/17 15:29, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
> > > Hi,
> > >
> > > On 2023/4/14 03:16, Thomas Zimmermann wrote:
> > > > 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@loongson.cn>
> > > > >
> > > > > The crazy fbdev test of IGT may write after EOF, which lead
> > > > > to out-of-bound
> > > >
> > > > Please drop 'crazy'. :)
> > >
> > > This is OK.
> > >
> > > By using the world 'crazy',
> > >
> > > I meant that the test is very good and maybe it is written by
> > > professional peoples
> > >
> > > with the guidance by experienced engineer. So that even the corner
> > > get tested.
> > >
> > >
> > > >
> > > > > 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@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.
> > >
> > >
> > > Are you meant that we should remove all these changes in
> > > drivers/gpu/drm/drm_fb_helper.c ?
> >
> > As Daniel mentioned, there's the discussion in the other thread. I don't
> > want to reopen it here. Just to summarize: I'm not convinced that this
> > should be DRM code because it can be shared with other fbdev drivers.
> >
> > [...]
> >
> > > > > 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];
> >
> > This has been bothering me over the weekend. And I think it's because
> > what we want the screen_size to be heigth * pitch, but the mmap'ed
> > memory is still at page granularity. Therefore...
> >
> > [...]
> > > >
> > > > > screen_buffer = vzalloc(screen_size);
> >
> > ... this line should explicitly allocate multiples of pages. Something
> > like
> >
> > /* allocate page-size multiples for mmap */
> > vzalloc(PAGE_ALIGN(screen_size))
> >
> I just thought about your instruction at here, thanks!
>
> But it is already page size aligned if we don't tough it.
>
> It is guaranteed by the GEM memory manger,
>
> so a previous patch tested by me, is turn out to be a extremely correct?
>
> We exposed a page size aligned buffer(even though it is larger than needed)
> is actually for mmap ?
mmap() is always page aligned, because that's the smallest size the cpu
pagetables can map. So there's fundamentally always a bit of memory at the
end of the buffer which logically is not part of the framebuffer memory.
And somehow we need to handle that to make sure we don't overflow.
-Daniel
>
>
> > It has not been a bug so far because vzalloc() always returns full pages
> > IIRC. It's still worth fixing.
> >
>
> > Best regards
> > Thomas
> >
> >
> > > > > if (!screen_buffer) {
> > > > > ret = -ENOMEM;
> > > >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/fbdev-generic: prohibit potential out-of-bounds access
2023-04-27 9:34 ` Daniel Vetter
(?)
@ 2023-04-28 8:08 ` Sui Jingfeng
-1 siblings, 0 replies; 20+ messages in thread
From: Sui Jingfeng @ 2023-04-28 8:08 UTC (permalink / raw)
To: Sui Jingfeng, Thomas Zimmermann, Maxime Ripard, David Airlie,
Li Yi, Javier Martinez Canillas, Helge Deller, Lucas De Marchi,
linux-kernel, linux-fbdev, dri-devel, loongson-kernel
Hi,
On 2023/4/27 17:34, Daniel Vetter wrote:
> On Thu, Apr 20, 2023 at 02:47:24AM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2023/4/17 15:29, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 14.04.23 um 12:58 schrieb Sui Jingfeng:
>>>> Hi,
>>>>
>>>> On 2023/4/14 03:16, Thomas Zimmermann wrote:
>>>>> 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@loongson.cn>
>>>>>>
>>>>>> The crazy fbdev test of IGT may write after EOF, which lead
>>>>>> to out-of-bound
>>>>> Please drop 'crazy'. :)
>>>> This is OK.
>>>>
>>>> By using the world 'crazy',
>>>>
>>>> I meant that the test is very good and maybe it is written by
>>>> professional peoples
>>>>
>>>> with the guidance by experienced engineer. So that even the corner
>>>> get tested.
>>>>
>>>>
>>>>>> 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@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.
>>>>
>>>> Are you meant that we should remove all these changes in
>>>> drivers/gpu/drm/drm_fb_helper.c ?
>>> As Daniel mentioned, there's the discussion in the other thread. I don't
>>> want to reopen it here. Just to summarize: I'm not convinced that this
>>> should be DRM code because it can be shared with other fbdev drivers.
>>>
>>> [...]
>>>
>>>>>> 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];
>>> This has been bothering me over the weekend. And I think it's because
>>> what we want the screen_size to be heigth * pitch, but the mmap'ed
>>> memory is still at page granularity. Therefore...
>>>
>>> [...]
>>>>>> screen_buffer = vzalloc(screen_size);
>>> ... this line should explicitly allocate multiples of pages. Something
>>> like
>>>
>>> /* allocate page-size multiples for mmap */
>>> vzalloc(PAGE_ALIGN(screen_size))
>>>
>> I just thought about your instruction at here, thanks!
>>
>> But it is already page size aligned if we don't tough it.
>>
>> It is guaranteed by the GEM memory manger,
>>
>> so a previous patch tested by me, is turn out to be a extremely correct?
>>
>> We exposed a page size aligned buffer(even though it is larger than needed)
>> is actually for mmap ?
> mmap() is always page aligned, because that's the smallest size the cpu
> pagetables can map. So there's fundamentally always a bit of memory at the
> end of the buffer which logically is not part of the framebuffer memory.
> And somehow we need to handle that to make sure we don't overflow.
> -Daniel
Yeah, buffer allocating should be page size aligned,
A single page share the same caching property.
fbdev test use the `smem_len` member of `fix_info` to know the true size
of the shadow screen buffer.
Exposing a large one actually allow the test writing to somewhere beyond
the logically visible area.
I need to learn more and testing more to verify if there are still
risks, I'll take a look.
Thanks for sharing the knowledge.
>>
>>> It has not been a bug so far because vzalloc() always returns full pages
>>> IIRC. It's still worth fixing.
>>>
>>> Best regards
>>> Thomas
>>>
>>>
>>>>>> if (!screen_buffer) {
>>>>>> ret = -ENOMEM;
^ permalink raw reply [flat|nested] 20+ messages in thread