Here are the fields that are set in the C reproducer for this ioctl:
#define FBIOPUT_VSCREENINFO 0x4601
*(uint32_t*)0x20000200 = 0x356; // xres: 854
*(uint32_t*)0x20000204 = 8; // yres
*(uint32_t*)0x20000208 = 0x600; // xres_virtual: 1536
*(uint32_t*)0x2000020c = 0x10000000; // yres_virtual: huge
*(uint32_t*)0x20000210 = 0; // xoffset
*(uint32_t*)0x20000214 = 0; // yoffset
*(uint32_t*)0x20000218 = 4; // bits_per_pixel
*(uint32_t*)0x2000021c = 0; // grayscale: false
*(uint32_t*)0x20000220 = 0x3000000; // red bitfield
*(uint32_t*)0x20000224 = 0; // green bitfield
*(uint32_t*)0x20000228 = 0; // blue bitfield
*(uint32_t*)0x2000022c = 0; // transp
*(uint32_t*)0x20000230 = 0; // nonstd: false
*(uint32_t*)0x20000234 = 0; // activate
*(uint32_t*)0x20000238 = 0; // height
*(uint32_t*)0x2000023c = 0; // width
*(uint32_t*)0x20000240 = 0; // accel_flags
*(uint32_t*)0x20000244 = 0; // pixclock
*(uint32_t*)0x20000248 = 0; // left_margin
*(uint32_t*)0x2000024c = 0; // right_margin
*(uint32_t*)0x20000250 = 0; // upper_margin
*(uint32_t*)0x20000254 = 0; // lower_margin
*(uint32_t*)0x20000258 = 0; // hsync_len
*(uint32_t*)0x2000025c = 0; // vsync_len
*(uint32_t*)0x20000260 = 0; // sync
*(uint32_t*)0x20000264 = 0; // vmode
*(uint32_t*)0x20000268 = 0; // rotate
*(uint32_t*)0x2000026c = 0; // colorspace
*(uint32_t*)0x20000270 = 0; // rsvd0
*(uint32_t*)0x20000274 = 0; // rsvd1
*(uint32_t*)0x20000278 = 0; // rsvd2
*(uint32_t*)0x2000027c = 0; // rsvd3
*(uint32_t*)0x20000280 = 0; // notdef...
*(uint32_t*)0x20000284 = 0;
*(uint32_t*)0x20000288 = 0;
*(uint32_t*)0x2000028c = 0;
memset((void*)0x20000290, 0, 16);
syscall(__NR_ioctl, r[0], 0x4601, 0x20000200ul);
Note that yres_virtual is set to 0x10000000. Is there no practical limit
(hence limit check) that can be used here?
Also, in vga16fb_check_var(), beginning at line 404:
404 if (yres > vyres)
405 vyres = yres;
406 if (vxres * vyres > maxmem) {
407 vyres = maxmem / vxres;
408 if (vyres < yres)
409 return -ENOMEM;
410 }
At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this
case/example), so any protection from this block is lost.
But even if yres_virtual (aka vyres) is "only" 0x01000000, so no
multiplication overflow occurs, the resulting value of vyres "seems"
to still be too large and can cause an error [I'm not sure about this
last part -- I need to use a new gcc so that KASAN will work.]
Note that yres_virtual is set to 0x10000000. Is there no practical limit
(hence limit check) that can be used here?
Also, in vga16fb_check_var(), beginning at line 404:
404 if (yres > vyres)
405 vyres = yres;
406 if (vxres * vyres > maxmem) {
407 vyres = maxmem / vxres;
408 if (vyres < yres)
409 return -ENOMEM;
410 }
At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this
case/example), so any protection from this block is lost.
But even if yres_virtual (aka vyres) is "only" 0x01000000, so no
multiplication overflow occurs, the resulting value of vyres "seems"
to still be too large and can cause an error [I'm not sure about this
last part -- I need to use a new gcc so that KASAN will work.]
From: Randy Dunlap <hidden> Date: 2021-08-30 02:31:58
On 8/29/21 7:27 PM, Tetsuo Handa wrote:
quoted hunk
On 2021/08/30 9:24, Randy Dunlap wrote:
quoted
Note that yres_virtual is set to 0x10000000. Is there no practical limit
(hence limit check) that can be used here?
Also, in vga16fb_check_var(), beginning at line 404:
404 if (yres > vyres)
405 vyres = yres;
406 if (vxres * vyres > maxmem) {
407 vyres = maxmem / vxres;
408 if (vyres < yres)
409 return -ENOMEM;
410 }
At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this
case/example), so any protection from this block is lost.
But even if yres_virtual (aka vyres) is "only" 0x01000000, so no
multiplication overflow occurs, the resulting value of vyres "seems"
to still be too large and can cause an error [I'm not sure about this
last part -- I need to use a new gcc so that KASAN will work.]
Hi Testsuo,
On Mon, Aug 30, 2021 at 4:27 AM Tetsuo Handa
[off-list ref] wrote:
quoted hunk
On 2021/08/30 9:24, Randy Dunlap wrote:
quoted
Note that yres_virtual is set to 0x10000000. Is there no practical limit
(hence limit check) that can be used here?
Also, in vga16fb_check_var(), beginning at line 404:
404 if (yres > vyres)
405 vyres = yres;
406 if (vxres * vyres > maxmem) {
407 vyres = maxmem / vxres;
408 if (vyres < yres)
409 return -ENOMEM;
410 }
At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this
case/example), so any protection from this block is lost.
@@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var,if(yres>vyres)vyres=yres;-if(vxres*vyres>maxmem){+if((u64)vxres*vyres>(u64)maxmem){
Mindlessly changing the sizes is not the solution.
Please use e.g. the array_size() helper from <linux/overflow.h>
instead.
quoted hunk
vyres = maxmem / vxres;
if (vyres < yres)
return -ENOMEM;
But I think we can check overflow in the common code like below. (Both patch fixed the oops.)
Same comment here, of course.
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
From: Dan Carpenter <hidden> Date: 2021-08-30 13:00:48
On Mon, Aug 30, 2021 at 02:00:21PM +0200, Geert Uytterhoeven wrote:
Hi Testsuo,
On Mon, Aug 30, 2021 at 4:27 AM Tetsuo Handa
[off-list ref] wrote:
quoted
On 2021/08/30 9:24, Randy Dunlap wrote:
quoted
Note that yres_virtual is set to 0x10000000. Is there no practical limit
(hence limit check) that can be used here?
Also, in vga16fb_check_var(), beginning at line 404:
404 if (yres > vyres)
405 vyres = yres;
406 if (vxres * vyres > maxmem) {
407 vyres = maxmem / vxres;
408 if (vyres < yres)
409 return -ENOMEM;
410 }
At line 406, the product of vxres * vyres overflows 32 bits (is 0 in this
case/example), so any protection from this block is lost.
@@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var,if(yres>vyres)vyres=yres;-if(vxres*vyres>maxmem){+if((u64)vxres*vyres>(u64)maxmem){
Mindlessly changing the sizes is not the solution.
Please use e.g. the array_size() helper from <linux/overflow.h>
instead.
On a 64bit system the array_size() macro is going to do the exact same
casts? But I do think this code would be easier to understand if the
integer overflow check were pull out separately and done first:
if (array_size(vxres, vyres) >= UINT_MAX)
return -EINVAL;
if (vxres * vyres > maxmem) {
...
The UINT_MAX is because vxres and vyres are u32.
This would maybe be the first time anyone ever did an integer overflow
check like this in the kernel. It's a new idiom.
regards,
dan carpenter
@@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var,if(yres>vyres)vyres=yres;-if(vxres*vyres>maxmem){+if((u64)vxres*vyres>(u64)maxmem){
Mindlessly changing the sizes is not the solution.
Please use e.g. the array_size() helper from <linux/overflow.h>
instead.
On a 64bit system the array_size() macro is going to do the exact same
casts? But I do think this code would be easier to understand if the
integer overflow check were pull out separately and done first:
if (array_size(vxres, vyres) >= UINT_MAX)
return -EINVAL;
This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and
returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid
value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15).
Comparing like "> (u64) UINT_MAX" is to detect only overflow.
array_size() would be helpful for forcing memory allocation to fail
(instead of allocating smaller than actually required).
if (vxres * vyres > maxmem) {
...
The UINT_MAX is because vxres and vyres are u32.
This would maybe be the first time anyone ever did an integer overflow
check like this in the kernel. It's a new idiom.
regards,
dan carpenter
@@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var,if(yres>vyres)vyres=yres;-if(vxres*vyres>maxmem){+if((u64)vxres*vyres>(u64)maxmem){
Mindlessly changing the sizes is not the solution.
Please use e.g. the array_size() helper from <linux/overflow.h>
instead.
On a 64bit system the array_size() macro is going to do the exact same
casts? But I do think this code would be easier to understand if the
integer overflow check were pull out separately and done first:
if (array_size(vxres, vyres) >= UINT_MAX)
return -EINVAL;
This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and
returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid
value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15).
Huh... I just assumed we didn't allow resolutions that high.
Comparing like "> (u64) UINT_MAX" is to detect only overflow.
Of course, that doesn't work on 32 bit systems. Also the cast isn't
required because of type promotion.
regards,
dan carpenter
@@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var,if(yres>vyres)vyres=yres;-if(vxres*vyres>maxmem){+if((u64)vxres*vyres>(u64)maxmem){
Mindlessly changing the sizes is not the solution.
Please use e.g. the array_size() helper from <linux/overflow.h>
instead.
On a 64bit system the array_size() macro is going to do the exact same
casts? But I do think this code would be easier to understand if the
integer overflow check were pull out separately and done first:
if (array_size(vxres, vyres) >= UINT_MAX)
return -EINVAL;
This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and
returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid
value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15).
Huh... I just assumed we didn't allow resolutions that high.
Of course, we don't allow resolutions that high. ;-)
Since I don't know possible max resolutions, I chose UINT_MAX + 1 as a common
limit for returning -EINVAL. Unless overflow happens, vga16fb_check_var() will
return -ENOMEM on such high resolutions.
quoted
Comparing like "> (u64) UINT_MAX" is to detect only overflow.
Of course, that doesn't work on 32 bit systems. Also the cast isn't
required because of type promotion.
Indeed, "> UINT_MAX" seems to work on both 32bits and 64bits.
----------
#include <stdio.h>
#include <limits.h>
int main(int argc, char *argv[])
{
unsigned int w = 0x600;
unsigned int h = 0x10000000;
if ((unsigned long long) w * h > UINT_MAX)
printf("Overflowed\n");
else
printf("No overflow\n");
return 0;
}
----------
@@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var,if(yres>vyres)vyres=yres;-if(vxres*vyres>maxmem){+if((u64)vxres*vyres>(u64)maxmem){
Mindlessly changing the sizes is not the solution.
Please use e.g. the array_size() helper from <linux/overflow.h>
instead.
On a 64bit system the array_size() macro is going to do the exact same
casts? But I do think this code would be easier to understand if the
integer overflow check were pull out separately and done first:
if (array_size(vxres, vyres) >= UINT_MAX)
return -EINVAL;
This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and
returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid
value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15).
Huh... I just assumed we didn't allow resolutions that high.
Of course, we don't allow resolutions that high. ;-)
Since I don't know possible max resolutions, I chose UINT_MAX + 1 as a common
limit for returning -EINVAL. Unless overflow happens, vga16fb_check_var() will
return -ENOMEM on such high resolutions.
quoted
quoted
Comparing like "> (u64) UINT_MAX" is to detect only overflow.
Of course, that doesn't work on 32 bit systems. Also the cast isn't
required because of type promotion.
Indeed, "> UINT_MAX" seems to work on both 32bits and 64bits.
Sorry, for the confusion. I'm talking about array_size() which is
size_t. Your approach using unsigned long long works.
regards,
dan carpenter
@@ -403,7 +403,7 @@ static int vga16fb_check_var(struct fb_var_screeninfo *var,if(yres>vyres)vyres=yres;-if(vxres*vyres>maxmem){+if((u64)vxres*vyres>(u64)maxmem){
Mindlessly changing the sizes is not the solution.
Please use e.g. the array_size() helper from <linux/overflow.h>
instead.
On a 64bit system the array_size() macro is going to do the exact same
casts? But I do think this code would be easier to understand if the
integer overflow check were pull out separately and done first:
if (array_size(vxres, vyres) >= UINT_MAX)
return -EINVAL;
This is wrong. array_size() returns ULONG_MAX on 64bits upon overflow and
returns UINT_MAX on 32bits upon overflow. However, UINT_MAX is a valid
value without overflow (e.g. vxres == UINT_MAX / 15 && vyres == 15).
Huh... I just assumed we didn't allow resolutions that high.
Of course, we don't allow resolutions that high. ;-)
Since I don't know possible max resolutions, I chose UINT_MAX + 1 as a common
limit for returning -EINVAL. Unless overflow happens, vga16fb_check_var() will
return -ENOMEM on such high resolutions.
The highest possible value of maxmem inside vga16fb_check_var()
is 65536.
So I believe
if (array_size(vxres, vyres) > maxmem)
should work fine.
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
Which I believe is wrong.
Thanks for the pointer, I will reply to the actual patch...
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
Which I believe is wrong.
Thanks for the pointer, I will reply to the actual patch...
Upon second look, that patch is not really wrong, as the check happens
after calling into info->fbops->fb_check_var().
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
syzbot is reporting page fault at vga16fb_fillrect() [1], for
vga16fb_check_var() is failing to detect multiplication overflow.
if (vxres * vyres > maxmem) {
vyres = maxmem / vxres;
if (vyres < yres)
return -ENOMEM;
}
Since no module would accept too huge resolutions where multiplication
overflow happens, let's reject in the common path.
This patch does not use array_size(), for array_size() is allowed to
return UINT_MAX on 32bits even if overflow did not happen. We want to
detect only overflow here, for individual module will recheck with more
strict limits as needed.
Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1]
Reported-by: syzbot <redacted>
Debugged-by: Randy Dunlap [off-list ref]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <redacted>
---
drivers/video/fbdev/core/fbmem.c | 5 +++++
1 file changed, 5 insertions(+)
Hi Tetsuo,
Thanks for your patch!
On Mon, Aug 30, 2021 at 6:05 PM Tetsuo Handa
[off-list ref] wrote:
syzbot is reporting page fault at vga16fb_fillrect() [1], for
vga16fb_check_var() is failing to detect multiplication overflow.
if (vxres * vyres > maxmem) {
vyres = maxmem / vxres;
if (vyres < yres)
return -ENOMEM;
}
IMHO that should be fixed in vga16fb, too.
Since no module would accept too huge resolutions where multiplication
overflow happens, let's reject in the common path.
This patch does not use array_size(), for array_size() is allowed to
return UINT_MAX on 32bits even if overflow did not happen. We want to
detect only overflow here, for individual module will recheck with more
strict limits as needed.
Which is IMHO not really an issue, as I believe on 32-bit you cannot
use a very large frame buffer, long before you reach UINT_MAX.
I think it would still be better to use check_mul_overflow(), as that
makes it clear and explicit what is being done, even without a comment.
Furthermore, this restricts the virtual frame buffer size on 64-bit,
too, while graphics cards can have much more than 4 GiB of RAM.
ret = info->fbops->fb_check_var(var, info);
if (ret)
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
Furthermore, this restricts the virtual frame buffer size on 64-bit,
too, while graphics cards can have much more than 4 GiB of RAM.
Excuse me, but do you mean that some hardware allows allocating more than
UINT_MAX bytes of memory for kernel frame buffer drivers?
IMHO that should be fixed in vga16fb, too.
According to https://elixir.bootlin.com/linux/v5.14/A/ident/fb_check_var ,
there are 89 files. Randomly picking up drivers/video/fbdev/udlfb.c as
an example. dlfb_is_valid_mode() from dlfb_ops_check_var() is doing
if (mode->xres * mode->yres > dlfb->sku_pixel_limit)
return 0;
return 1;
where max dlfb->sku_pixel_limit seems to be 2048 * 1152 but I think we need
same overflow check. I want to avoid patching individual modules if possible.
That depends on whether some hardware needs to allocate more than UINT_MAX
bytes of memory.
From: Daniel Vetter <hidden> Date: 2021-08-31 16:20:23
On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
[off-list ref] wrote:
On 2021/08/31 15:48, Geert Uytterhoeven wrote:
quoted
Furthermore, this restricts the virtual frame buffer size on 64-bit,
too, while graphics cards can have much more than 4 GiB of RAM.
Excuse me, but do you mean that some hardware allows allocating more than
UINT_MAX bytes of memory for kernel frame buffer drivers?
quoted
IMHO that should be fixed in vga16fb, too.
According to https://elixir.bootlin.com/linux/v5.14/A/ident/fb_check_var ,
there are 89 files. Randomly picking up drivers/video/fbdev/udlfb.c as
an example. dlfb_is_valid_mode() from dlfb_ops_check_var() is doing
if (mode->xres * mode->yres > dlfb->sku_pixel_limit)
return 0;
return 1;
where max dlfb->sku_pixel_limit seems to be 2048 * 1152 but I think we need
same overflow check. I want to avoid patching individual modules if possible.
That depends on whether some hardware needs to allocate more than UINT_MAX
bytes of memory.
Yeah basic input validation makes no sense to push into each driver.
That's just asking that most of the fbdev drivers will never be fixed.
Same for not-so-basic input validation, if there's no driver that
actually needs the flexibility (like the virtual vs physical size
thing that's floating around maybe).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Handa-san,
On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
[off-list ref] wrote:
On 2021/08/31 15:48, Geert Uytterhoeven wrote:
quoted
Furthermore, this restricts the virtual frame buffer size on 64-bit,
too, while graphics cards can have much more than 4 GiB of RAM.
Excuse me, but do you mean that some hardware allows allocating more than
UINT_MAX bytes of memory for kernel frame buffer drivers?
While smem_len is u32 (there have been complaints about such
limitations on 64-bit platforms as far as 10 years ago), I see no
reason why a graphics card with more than 4 GiB of RAM would not be
able to provide a very large virtual screen.
Of course e.g. vga16fb cannot, as it is limited to 64 KiB.
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
From: Daniel Vetter <hidden> Date: 2021-08-31 18:53:36
On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven [off-list ref] wrote:
Hi Handa-san,
On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
[off-list ref] wrote:
quoted
On 2021/08/31 15:48, Geert Uytterhoeven wrote:
quoted
Furthermore, this restricts the virtual frame buffer size on 64-bit,
too, while graphics cards can have much more than 4 GiB of RAM.
Excuse me, but do you mean that some hardware allows allocating more than
UINT_MAX bytes of memory for kernel frame buffer drivers?
While smem_len is u32 (there have been complaints about such
limitations on 64-bit platforms as far as 10 years ago), I see no
reason why a graphics card with more than 4 GiB of RAM would not be
able to provide a very large virtual screen.
Of course e.g. vga16fb cannot, as it is limited to 64 KiB.
The first gpus with 4GB or more memory started shipping in 2012. We're
not going to have fbdev drivers for these, so let's not invent code
for use-cases that aren't please.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Daniel,
On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter [off-list ref] wrote:
On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
[off-list ref] wrote:
quoted
On 2021/08/31 15:48, Geert Uytterhoeven wrote:
quoted
Furthermore, this restricts the virtual frame buffer size on 64-bit,
too, while graphics cards can have much more than 4 GiB of RAM.
Excuse me, but do you mean that some hardware allows allocating more than
UINT_MAX bytes of memory for kernel frame buffer drivers?
While smem_len is u32 (there have been complaints about such
limitations on 64-bit platforms as far as 10 years ago), I see no
reason why a graphics card with more than 4 GiB of RAM would not be
able to provide a very large virtual screen.
Of course e.g. vga16fb cannot, as it is limited to 64 KiB.
The first gpus with 4GB or more memory started shipping in 2012. We're
not going to have fbdev drivers for these, so let's not invent code
for use-cases that aren't please.
This code path is used with fbdev emulation for drm drivers, too,
right?
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
From: Daniel Vetter <hidden> Date: 2021-08-31 19:05:13
On Tue, Aug 31, 2021 at 8:56 PM Geert Uytterhoeven [off-list ref] wrote:
Hi Daniel,
On Tue, Aug 31, 2021 at 8:53 PM Daniel Vetter [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 7:19 PM Geert Uytterhoeven [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 5:24 PM Tetsuo Handa
[off-list ref] wrote:
quoted
On 2021/08/31 15:48, Geert Uytterhoeven wrote:
quoted
Furthermore, this restricts the virtual frame buffer size on 64-bit,
too, while graphics cards can have much more than 4 GiB of RAM.
Excuse me, but do you mean that some hardware allows allocating more than
UINT_MAX bytes of memory for kernel frame buffer drivers?
While smem_len is u32 (there have been complaints about such
limitations on 64-bit platforms as far as 10 years ago), I see no
reason why a graphics card with more than 4 GiB of RAM would not be
able to provide a very large virtual screen.
Of course e.g. vga16fb cannot, as it is limited to 64 KiB.
The first gpus with 4GB or more memory started shipping in 2012. We're
not going to have fbdev drivers for these, so let's not invent code
for use-cases that aren't please.
This code path is used with fbdev emulation for drm drivers, too,
right?
Yeah, you get one buffer, with overallocating 2. That's all, you don't
get the entire vram because we can't revoke that for fbdev users. We'd
have fixed this long ago if it's a real limitations.
8k at 64bpp is still less than 256MB. Also due to pci bar limitations
(which finally get lifted now because windows fixed their pci code,
which motivates motherboard manufactures for desktop space to also fix
theirs) we're limited to 256MB actually cpu visible anyway.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
syzbot is reporting page fault at vga16fb_fillrect() [1], for
vga16fb_check_var() is failing to detect multiplication overflow.
if (vxres * vyres > maxmem) {
vyres = maxmem / vxres;
if (vyres < yres)
return -ENOMEM;
}
Since no module would accept too huge resolutions where multiplication
overflow happens, let's reject in the common path.
Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1]
Reported-by: syzbot <redacted>
Debugged-by: Randy Dunlap [off-list ref]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v2:
Use check_mul_overflow(), suggested by Geert Uytterhoeven [off-list ref].
drivers/video/fbdev/core/fbmem.c | 6 ++++++
1 file changed, 6 insertions(+)
On Wed, Sep 1, 2021 at 3:15 AM Tetsuo Handa
[off-list ref] wrote:
syzbot is reporting page fault at vga16fb_fillrect() [1], for
vga16fb_check_var() is failing to detect multiplication overflow.
if (vxres * vyres > maxmem) {
vyres = maxmem / vxres;
if (vyres < yres)
return -ENOMEM;
}
Since no module would accept too huge resolutions where multiplication
overflow happens, let's reject in the common path.
Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1]
Reported-by: syzbot <redacted>
Debugged-by: Randy Dunlap [off-list ref]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-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
syzbot is reporting page fault at vga16fb_fillrect() [1], for
vga16fb_check_var() is failing to detect multiplication overflow.
if (vxres * vyres > maxmem) {
vyres = maxmem / vxres;
if (vyres < yres)
return -ENOMEM;
}
Since no module would accept too huge resolutions where multiplication
overflow happens, let's reject in the common path.
Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1]
Reported-by: syzbot <redacted>
Debugged-by: Randy Dunlap [off-list ref]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Changes in v2:
Use check_mul_overflow(), suggested by Geert Uytterhoeven [off-list ref].
drivers/video/fbdev/core/fbmem.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Daniel Vetter <hidden> Date: 2021-09-08 16:52:08
On Wed, Sep 08, 2021 at 07:27:49PM +0900, Tetsuo Handa wrote:
syzbot is reporting page fault at vga16fb_fillrect() [1], for
vga16fb_check_var() is failing to detect multiplication overflow.
if (vxres * vyres > maxmem) {
vyres = maxmem / vxres;
if (vyres < yres)
return -ENOMEM;
}
Since no module would accept too huge resolutions where multiplication
overflow happens, let's reject in the common path.
Link: https://syzkaller.appspot.com/bug?extid=04168c8063cfdde1db5e [1]
Reported-by: syzbot <redacted>
Debugged-by: Randy Dunlap [off-list ref]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Changes in v2:
Use check_mul_overflow(), suggested by Geert Uytterhoeven [off-list ref].
Pushed to drm-misc-next-fixes so it should get into current merge window.
I also added a cc: stable here, I htink it's needed.
Thanks a lot to both you&Geert for handling this!
-Daniel