* [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-12 11:10 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-12 11:10 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Dmitry Vyukov, dri-devel, linux-fbdev, linux-kernel, Tetsuo Handa
I found that
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 1;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
causes general protection fault in bitfill_aligned(), for vc_do_resize()
updates vc->vc_{cols,rows} only when vc_do_resize() will return 0.
[ 20.102222] BUG: unable to handle page fault for address: ffffb80500d7b000
[ 20.102225] #PF: supervisor write access in kernel mode
[ 20.102226] #PF: error_code(0x0002) - not-present page
[ 20.102227] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 132525067 PTE 0
[ 20.102230] Oops: 0002 [#1] SMP
[ 20.102232] CPU: 3 PID: 2786 Comm: a.out Not tainted 5.8.0-rc4+ #749
[ 20.102233] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[ 20.102237] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
[ 20.102277] Call Trace:
[ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
[ 20.102747] vmw_fb_fillrect+0x12/0x30 [vmwgfx]
[ 20.102755] bit_clear_margins+0x92/0xf0 [fb]
[ 20.102760] fbcon_clear_margins+0x4c/0x50 [fb]
[ 20.102763] fbcon_switch+0x321/0x570 [fb]
[ 20.102771] redraw_screen+0xe0/0x250
[ 20.102775] fbcon_modechanged+0x164/0x1b0 [fb]
[ 20.102779] fbcon_update_vcs+0x15/0x20 [fb]
[ 20.102781] fb_set_var+0x364/0x3c0 [fb]
[ 20.102817] do_fb_ioctl+0x2ff/0x3f0 [fb]
[ 20.103139] fb_ioctl+0x2e/0x40 [fb]
[ 20.103141] ksys_ioctl+0x86/0xc0
[ 20.103146] __x64_sys_ioctl+0x15/0x20
[ 20.103148] do_syscall_64+0x54/0xa0
[ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
If vc_do_resize() fails (e.g. kzalloc() failure) when var.xres or var.yres
is going to shrink, bit_clear_margins() hits integer underflow bug due to
info->var.xres < (vc->vc_cols * cw) or info->var.yres < (vc->vc_rows * ch).
Unexpectedly large rw or bh will try to overrun the __iomem region and
causes general protection fault.
This crash is easily reproducible by calling vc_do_resize(vc, 0, 0)
which the reproducer above will do. Since fbcon_modechanged() is doing
cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
cols /= vc->vc_font.width;
rows /= vc->vc_font.height;
vc_resize(vc, cols, rows);
(...snipped...)
update_screen(vc);
, var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height
makes rows = 0. But vc_do_resize() does not set vc->vc_cols = vc->vc_rows = 0
due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception.
Of course, the root problem is that callers of do_vc_resize() are not
handling vc_do_resize() failures, but it might not be easy to handle
them under complicated dependency. Therefore, as a band-aid workaround,
this patch checks integer underflow in "struct fbcon_ops"->clear_margins
call, assuming that vc->vc_cols * vc->vc_font.width and
vc->vc_rows * vc->vc_font.heigh do not cause integer overflow.
I hope that we can survive even if info->var.{xres,yres} were increased
but vc->vc_{cols,rows} were not increased due to kzalloc() failure, for
the __iomem memory for cfb_fillrect() seems to be allocated upon driver
load.
By the way, syzbot has several reports which are stalling inside filling
functions. Although reproducer for [1] is not found yet, it has tried
r0 = openat$fb0(0xffffffffffffff9c, &(0x7f0000000180)='/dev/fb0\x00', 0x0, 0x0)
ioctl$FBIOPUT_VSCREENINFO(r0, 0x4601, &(0x7f0000000000)={0x0, 0x0, 0x0, 0x500, 0x0, 0x0, 0x4})
which corresponds to
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
var.yres_virtual = 0x500;
var.bits_per_pixel = 4;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
and somehow hit unexpectedly long bit_clear_margins() loops. I don't know
why syzbot does not hit general protection fault, but it would depend on
environment because in my VMware environment ioctl(FBIOPUT_VSCREENINFO)
returns -EINVAL if var.xres == var.yres == 0.
[1] https://syzkaller.appspot.com/bug?id=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/video/fbdev/core/bitblit.c | 4 ++--
drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..35ebeeccde4d 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
region.width = rw;
@@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset + bs;
region.width = rs;
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index dfa9a8aa4509..78f3a5621478 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset;
region.height = rw;
@@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset + bs;
region.dy = 0;
region.height = info->var.yres_virtual;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index ce08251bfd38..fd098ff17574 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset + rs;
region.height = rw;
@@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset;
region.height = info->var.yres;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 1936afc78fec..e165a3fad29a 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dy = 0;
region.dx = info->var.xoffset;
region.width = rw;
@@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dy = info->var.yoffset;
region.dx = info->var.xoffset;
region.height = bh;
--
2.18.4
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-12 11:10 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-12 11:10 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Tetsuo Handa, linux-fbdev, dri-devel, Dmitry Vyukov, linux-kernel
I found that
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 1;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
causes general protection fault in bitfill_aligned(), for vc_do_resize()
updates vc->vc_{cols,rows} only when vc_do_resize() will return 0.
[ 20.102222] BUG: unable to handle page fault for address: ffffb80500d7b000
[ 20.102225] #PF: supervisor write access in kernel mode
[ 20.102226] #PF: error_code(0x0002) - not-present page
[ 20.102227] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 132525067 PTE 0
[ 20.102230] Oops: 0002 [#1] SMP
[ 20.102232] CPU: 3 PID: 2786 Comm: a.out Not tainted 5.8.0-rc4+ #749
[ 20.102233] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[ 20.102237] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
[ 20.102277] Call Trace:
[ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
[ 20.102747] vmw_fb_fillrect+0x12/0x30 [vmwgfx]
[ 20.102755] bit_clear_margins+0x92/0xf0 [fb]
[ 20.102760] fbcon_clear_margins+0x4c/0x50 [fb]
[ 20.102763] fbcon_switch+0x321/0x570 [fb]
[ 20.102771] redraw_screen+0xe0/0x250
[ 20.102775] fbcon_modechanged+0x164/0x1b0 [fb]
[ 20.102779] fbcon_update_vcs+0x15/0x20 [fb]
[ 20.102781] fb_set_var+0x364/0x3c0 [fb]
[ 20.102817] do_fb_ioctl+0x2ff/0x3f0 [fb]
[ 20.103139] fb_ioctl+0x2e/0x40 [fb]
[ 20.103141] ksys_ioctl+0x86/0xc0
[ 20.103146] __x64_sys_ioctl+0x15/0x20
[ 20.103148] do_syscall_64+0x54/0xa0
[ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
If vc_do_resize() fails (e.g. kzalloc() failure) when var.xres or var.yres
is going to shrink, bit_clear_margins() hits integer underflow bug due to
info->var.xres < (vc->vc_cols * cw) or info->var.yres < (vc->vc_rows * ch).
Unexpectedly large rw or bh will try to overrun the __iomem region and
causes general protection fault.
This crash is easily reproducible by calling vc_do_resize(vc, 0, 0)
which the reproducer above will do. Since fbcon_modechanged() is doing
cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
cols /= vc->vc_font.width;
rows /= vc->vc_font.height;
vc_resize(vc, cols, rows);
(...snipped...)
update_screen(vc);
, var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height
makes rows = 0. But vc_do_resize() does not set vc->vc_cols = vc->vc_rows = 0
due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception.
Of course, the root problem is that callers of do_vc_resize() are not
handling vc_do_resize() failures, but it might not be easy to handle
them under complicated dependency. Therefore, as a band-aid workaround,
this patch checks integer underflow in "struct fbcon_ops"->clear_margins
call, assuming that vc->vc_cols * vc->vc_font.width and
vc->vc_rows * vc->vc_font.heigh do not cause integer overflow.
I hope that we can survive even if info->var.{xres,yres} were increased
but vc->vc_{cols,rows} were not increased due to kzalloc() failure, for
the __iomem memory for cfb_fillrect() seems to be allocated upon driver
load.
By the way, syzbot has several reports which are stalling inside filling
functions. Although reproducer for [1] is not found yet, it has tried
r0 = openat$fb0(0xffffffffffffff9c, &(0x7f0000000180)='/dev/fb0\x00', 0x0, 0x0)
ioctl$FBIOPUT_VSCREENINFO(r0, 0x4601, &(0x7f0000000000)={0x0, 0x0, 0x0, 0x500, 0x0, 0x0, 0x4})
which corresponds to
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
var.yres_virtual = 0x500;
var.bits_per_pixel = 4;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
and somehow hit unexpectedly long bit_clear_margins() loops. I don't know
why syzbot does not hit general protection fault, but it would depend on
environment because in my VMware environment ioctl(FBIOPUT_VSCREENINFO)
returns -EINVAL if var.xres == var.yres == 0.
[1] https://syzkaller.appspot.com/bug?id=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/video/fbdev/core/bitblit.c | 4 ++--
drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..35ebeeccde4d 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
region.width = rw;
@@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset + bs;
region.width = rs;
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index dfa9a8aa4509..78f3a5621478 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset;
region.height = rw;
@@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset + bs;
region.dy = 0;
region.height = info->var.yres_virtual;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index ce08251bfd38..fd098ff17574 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset + rs;
region.height = rw;
@@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset;
region.height = info->var.yres;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 1936afc78fec..e165a3fad29a 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dy = 0;
region.dx = info->var.xoffset;
region.width = rw;
@@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dy = info->var.yoffset;
region.dx = info->var.xoffset;
region.height = bh;
--
2.18.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-12 11:10 ` Tetsuo Handa
(?)
@ 2020-07-14 7:22 ` Bartlomiej Zolnierkiewicz
-1 siblings, 0 replies; 77+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-07-14 7:22 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
George Kennedy, Dan Carpenter, Jiri Slaby, Dmitry Vyukov
[ Please Cc: fbdev Maintainer (happens to be me :) on fbdev patches, thanks. ]
Hi,
On 7/12/20 1:10 PM, Tetsuo Handa wrote:
> I found that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> causes general protection fault in bitfill_aligned(), for vc_do_resize()
> updates vc->vc_{cols,rows} only when vc_do_resize() will return 0.
>
> [ 20.102222] BUG: unable to handle page fault for address: ffffb80500d7b000
> [ 20.102225] #PF: supervisor write access in kernel mode
> [ 20.102226] #PF: error_code(0x0002) - not-present page
> [ 20.102227] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 132525067 PTE 0
> [ 20.102230] Oops: 0002 [#1] SMP
> [ 20.102232] CPU: 3 PID: 2786 Comm: a.out Not tainted 5.8.0-rc4+ #749
> [ 20.102233] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
> [ 20.102237] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
> [ 20.102277] Call Trace:
> [ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
> [ 20.102747] vmw_fb_fillrect+0x12/0x30 [vmwgfx]
> [ 20.102755] bit_clear_margins+0x92/0xf0 [fb]
> [ 20.102760] fbcon_clear_margins+0x4c/0x50 [fb]
> [ 20.102763] fbcon_switch+0x321/0x570 [fb]
> [ 20.102771] redraw_screen+0xe0/0x250
> [ 20.102775] fbcon_modechanged+0x164/0x1b0 [fb]
> [ 20.102779] fbcon_update_vcs+0x15/0x20 [fb]
> [ 20.102781] fb_set_var+0x364/0x3c0 [fb]
> [ 20.102817] do_fb_ioctl+0x2ff/0x3f0 [fb]
> [ 20.103139] fb_ioctl+0x2e/0x40 [fb]
> [ 20.103141] ksys_ioctl+0x86/0xc0
> [ 20.103146] __x64_sys_ioctl+0x15/0x20
> [ 20.103148] do_syscall_64+0x54/0xa0
> [ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> If vc_do_resize() fails (e.g. kzalloc() failure) when var.xres or var.yres
> is going to shrink, bit_clear_margins() hits integer underflow bug due to
> info->var.xres < (vc->vc_cols * cw) or info->var.yres < (vc->vc_rows * ch).
> Unexpectedly large rw or bh will try to overrun the __iomem region and
> causes general protection fault.
>
> This crash is easily reproducible by calling vc_do_resize(vc, 0, 0)
> which the reproducer above will do. Since fbcon_modechanged() is doing
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
> (...snipped...)
> update_screen(vc);
>
> , var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height
> makes rows = 0. But vc_do_resize() does not set vc->vc_cols = vc->vc_rows = 0
> due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception.
>
> Of course, the root problem is that callers of do_vc_resize() are not
> handling vc_do_resize() failures, but it might not be easy to handle
> them under complicated dependency. Therefore, as a band-aid workaround,
> this patch checks integer underflow in "struct fbcon_ops"->clear_margins
> call, assuming that vc->vc_cols * vc->vc_font.width and
> vc->vc_rows * vc->vc_font.heigh do not cause integer overflow.
>
> I hope that we can survive even if info->var.{xres,yres} were increased
> but vc->vc_{cols,rows} were not increased due to kzalloc() failure, for
> the __iomem memory for cfb_fillrect() seems to be allocated upon driver
> load.
>
> By the way, syzbot has several reports which are stalling inside filling
> functions. Although reproducer for [1] is not found yet, it has tried
>
> r0 = openat$fb0(0xffffffffffffff9c, &(0x7f0000000180)='/dev/fb0\x00', 0x0, 0x0)
> ioctl$FBIOPUT_VSCREENINFO(r0, 0x4601, &(0x7f0000000000)={0x0, 0x0, 0x0, 0x500, 0x0, 0x0, 0x4})
>
> which corresponds to
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> var.yres_virtual = 0x500;
> var.bits_per_pixel = 4;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> and somehow hit unexpectedly long bit_clear_margins() loops. I don't know
> why syzbot does not hit general protection fault, but it would depend on
> environment because in my VMware environment ioctl(FBIOPUT_VSCREENINFO)
> returns -EINVAL if var.xres = var.yres = 0.
>
> [1] https://syzkaller.appspot.com/bug?id‘ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m\x159415024816722&w=2
?
It seems to address the same issue, I've added George and Dan to Cc:.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> region.width = rw;
> @@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset;
> region.dy = info->var.yoffset + bs;
> region.width = rs;
> diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
> index dfa9a8aa4509..78f3a5621478 100644
> --- a/drivers/video/fbdev/core/fbcon_ccw.c
> +++ b/drivers/video/fbdev/core/fbcon_ccw.c
> @@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = 0;
> region.dy = info->var.yoffset;
> region.height = rw;
> @@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset + bs;
> region.dy = 0;
> region.height = info->var.yres_virtual;
> diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
> index ce08251bfd38..fd098ff17574 100644
> --- a/drivers/video/fbdev/core/fbcon_cw.c
> +++ b/drivers/video/fbdev/core/fbcon_cw.c
> @@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = 0;
> region.dy = info->var.yoffset + rs;
> region.height = rw;
> @@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset;
> region.dy = info->var.yoffset;
> region.height = info->var.yres;
> diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
> index 1936afc78fec..e165a3fad29a 100644
> --- a/drivers/video/fbdev/core/fbcon_ud.c
> +++ b/drivers/video/fbdev/core/fbcon_ud.c
> @@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dy = 0;
> region.dx = info->var.xoffset;
> region.width = rw;
> @@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dy = info->var.yoffset;
> region.dx = info->var.xoffset;
> region.height = bh;
>
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-14 7:22 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 77+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-07-14 7:22 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-fbdev, dri-devel,
Dmitry Vyukov, linux-kernel, George Kennedy, Dan Carpenter
[ Please Cc: fbdev Maintainer (happens to be me :) on fbdev patches, thanks. ]
Hi,
On 7/12/20 1:10 PM, Tetsuo Handa wrote:
> I found that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> causes general protection fault in bitfill_aligned(), for vc_do_resize()
> updates vc->vc_{cols,rows} only when vc_do_resize() will return 0.
>
> [ 20.102222] BUG: unable to handle page fault for address: ffffb80500d7b000
> [ 20.102225] #PF: supervisor write access in kernel mode
> [ 20.102226] #PF: error_code(0x0002) - not-present page
> [ 20.102227] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 132525067 PTE 0
> [ 20.102230] Oops: 0002 [#1] SMP
> [ 20.102232] CPU: 3 PID: 2786 Comm: a.out Not tainted 5.8.0-rc4+ #749
> [ 20.102233] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
> [ 20.102237] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
> [ 20.102277] Call Trace:
> [ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
> [ 20.102747] vmw_fb_fillrect+0x12/0x30 [vmwgfx]
> [ 20.102755] bit_clear_margins+0x92/0xf0 [fb]
> [ 20.102760] fbcon_clear_margins+0x4c/0x50 [fb]
> [ 20.102763] fbcon_switch+0x321/0x570 [fb]
> [ 20.102771] redraw_screen+0xe0/0x250
> [ 20.102775] fbcon_modechanged+0x164/0x1b0 [fb]
> [ 20.102779] fbcon_update_vcs+0x15/0x20 [fb]
> [ 20.102781] fb_set_var+0x364/0x3c0 [fb]
> [ 20.102817] do_fb_ioctl+0x2ff/0x3f0 [fb]
> [ 20.103139] fb_ioctl+0x2e/0x40 [fb]
> [ 20.103141] ksys_ioctl+0x86/0xc0
> [ 20.103146] __x64_sys_ioctl+0x15/0x20
> [ 20.103148] do_syscall_64+0x54/0xa0
> [ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> If vc_do_resize() fails (e.g. kzalloc() failure) when var.xres or var.yres
> is going to shrink, bit_clear_margins() hits integer underflow bug due to
> info->var.xres < (vc->vc_cols * cw) or info->var.yres < (vc->vc_rows * ch).
> Unexpectedly large rw or bh will try to overrun the __iomem region and
> causes general protection fault.
>
> This crash is easily reproducible by calling vc_do_resize(vc, 0, 0)
> which the reproducer above will do. Since fbcon_modechanged() is doing
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
> (...snipped...)
> update_screen(vc);
>
> , var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height
> makes rows = 0. But vc_do_resize() does not set vc->vc_cols = vc->vc_rows = 0
> due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception.
>
> Of course, the root problem is that callers of do_vc_resize() are not
> handling vc_do_resize() failures, but it might not be easy to handle
> them under complicated dependency. Therefore, as a band-aid workaround,
> this patch checks integer underflow in "struct fbcon_ops"->clear_margins
> call, assuming that vc->vc_cols * vc->vc_font.width and
> vc->vc_rows * vc->vc_font.heigh do not cause integer overflow.
>
> I hope that we can survive even if info->var.{xres,yres} were increased
> but vc->vc_{cols,rows} were not increased due to kzalloc() failure, for
> the __iomem memory for cfb_fillrect() seems to be allocated upon driver
> load.
>
> By the way, syzbot has several reports which are stalling inside filling
> functions. Although reproducer for [1] is not found yet, it has tried
>
> r0 = openat$fb0(0xffffffffffffff9c, &(0x7f0000000180)='/dev/fb0\x00', 0x0, 0x0)
> ioctl$FBIOPUT_VSCREENINFO(r0, 0x4601, &(0x7f0000000000)={0x0, 0x0, 0x0, 0x500, 0x0, 0x0, 0x4})
>
> which corresponds to
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> var.yres_virtual = 0x500;
> var.bits_per_pixel = 4;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> and somehow hit unexpectedly long bit_clear_margins() loops. I don't know
> why syzbot does not hit general protection fault, but it would depend on
> environment because in my VMware environment ioctl(FBIOPUT_VSCREENINFO)
> returns -EINVAL if var.xres == var.yres == 0.
>
> [1] https://syzkaller.appspot.com/bug?id=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
?
It seems to address the same issue, I've added George and Dan to Cc:.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> region.width = rw;
> @@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset;
> region.dy = info->var.yoffset + bs;
> region.width = rs;
> diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
> index dfa9a8aa4509..78f3a5621478 100644
> --- a/drivers/video/fbdev/core/fbcon_ccw.c
> +++ b/drivers/video/fbdev/core/fbcon_ccw.c
> @@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = 0;
> region.dy = info->var.yoffset;
> region.height = rw;
> @@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset + bs;
> region.dy = 0;
> region.height = info->var.yres_virtual;
> diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
> index ce08251bfd38..fd098ff17574 100644
> --- a/drivers/video/fbdev/core/fbcon_cw.c
> +++ b/drivers/video/fbdev/core/fbcon_cw.c
> @@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = 0;
> region.dy = info->var.yoffset + rs;
> region.height = rw;
> @@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset;
> region.dy = info->var.yoffset;
> region.height = info->var.yres;
> diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
> index 1936afc78fec..e165a3fad29a 100644
> --- a/drivers/video/fbdev/core/fbcon_ud.c
> +++ b/drivers/video/fbdev/core/fbcon_ud.c
> @@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dy = 0;
> region.dx = info->var.xoffset;
> region.width = rw;
> @@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dy = info->var.yoffset;
> region.dx = info->var.xoffset;
> region.height = bh;
>
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-14 7:22 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 77+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-07-14 7:22 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
George Kennedy, Dan Carpenter, Jiri Slaby, Dmitry Vyukov
[ Please Cc: fbdev Maintainer (happens to be me :) on fbdev patches, thanks. ]
Hi,
On 7/12/20 1:10 PM, Tetsuo Handa wrote:
> I found that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> causes general protection fault in bitfill_aligned(), for vc_do_resize()
> updates vc->vc_{cols,rows} only when vc_do_resize() will return 0.
>
> [ 20.102222] BUG: unable to handle page fault for address: ffffb80500d7b000
> [ 20.102225] #PF: supervisor write access in kernel mode
> [ 20.102226] #PF: error_code(0x0002) - not-present page
> [ 20.102227] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 132525067 PTE 0
> [ 20.102230] Oops: 0002 [#1] SMP
> [ 20.102232] CPU: 3 PID: 2786 Comm: a.out Not tainted 5.8.0-rc4+ #749
> [ 20.102233] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
> [ 20.102237] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
> [ 20.102277] Call Trace:
> [ 20.102281] cfb_fillrect+0x159/0x340 [cfbfillrect]
> [ 20.102747] vmw_fb_fillrect+0x12/0x30 [vmwgfx]
> [ 20.102755] bit_clear_margins+0x92/0xf0 [fb]
> [ 20.102760] fbcon_clear_margins+0x4c/0x50 [fb]
> [ 20.102763] fbcon_switch+0x321/0x570 [fb]
> [ 20.102771] redraw_screen+0xe0/0x250
> [ 20.102775] fbcon_modechanged+0x164/0x1b0 [fb]
> [ 20.102779] fbcon_update_vcs+0x15/0x20 [fb]
> [ 20.102781] fb_set_var+0x364/0x3c0 [fb]
> [ 20.102817] do_fb_ioctl+0x2ff/0x3f0 [fb]
> [ 20.103139] fb_ioctl+0x2e/0x40 [fb]
> [ 20.103141] ksys_ioctl+0x86/0xc0
> [ 20.103146] __x64_sys_ioctl+0x15/0x20
> [ 20.103148] do_syscall_64+0x54/0xa0
> [ 20.103151] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> If vc_do_resize() fails (e.g. kzalloc() failure) when var.xres or var.yres
> is going to shrink, bit_clear_margins() hits integer underflow bug due to
> info->var.xres < (vc->vc_cols * cw) or info->var.yres < (vc->vc_rows * ch).
> Unexpectedly large rw or bh will try to overrun the __iomem region and
> causes general protection fault.
>
> This crash is easily reproducible by calling vc_do_resize(vc, 0, 0)
> which the reproducer above will do. Since fbcon_modechanged() is doing
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
> (...snipped...)
> update_screen(vc);
>
> , var.xres < vc->vc_font.width makes cols = 0 and var.yres < vc->vc_font.height
> makes rows = 0. But vc_do_resize() does not set vc->vc_cols = vc->vc_rows = 0
> due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception.
>
> Of course, the root problem is that callers of do_vc_resize() are not
> handling vc_do_resize() failures, but it might not be easy to handle
> them under complicated dependency. Therefore, as a band-aid workaround,
> this patch checks integer underflow in "struct fbcon_ops"->clear_margins
> call, assuming that vc->vc_cols * vc->vc_font.width and
> vc->vc_rows * vc->vc_font.heigh do not cause integer overflow.
>
> I hope that we can survive even if info->var.{xres,yres} were increased
> but vc->vc_{cols,rows} were not increased due to kzalloc() failure, for
> the __iomem memory for cfb_fillrect() seems to be allocated upon driver
> load.
>
> By the way, syzbot has several reports which are stalling inside filling
> functions. Although reproducer for [1] is not found yet, it has tried
>
> r0 = openat$fb0(0xffffffffffffff9c, &(0x7f0000000180)='/dev/fb0\x00', 0x0, 0x0)
> ioctl$FBIOPUT_VSCREENINFO(r0, 0x4601, &(0x7f0000000000)={0x0, 0x0, 0x0, 0x500, 0x0, 0x0, 0x4})
>
> which corresponds to
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> var.yres_virtual = 0x500;
> var.bits_per_pixel = 4;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> and somehow hit unexpectedly long bit_clear_margins() loops. I don't know
> why syzbot does not hit general protection fault, but it would depend on
> environment because in my VMware environment ioctl(FBIOPUT_VSCREENINFO)
> returns -EINVAL if var.xres == var.yres == 0.
>
> [1] https://syzkaller.appspot.com/bug?id=91ecc3bf32ab1a551c33a39dab7fc0c8cd7b7e16
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
How does this patch relate to:
https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
?
It seems to address the same issue, I've added George and Dan to Cc:.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> region.width = rw;
> @@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset;
> region.dy = info->var.yoffset + bs;
> region.width = rs;
> diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
> index dfa9a8aa4509..78f3a5621478 100644
> --- a/drivers/video/fbdev/core/fbcon_ccw.c
> +++ b/drivers/video/fbdev/core/fbcon_ccw.c
> @@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = 0;
> region.dy = info->var.yoffset;
> region.height = rw;
> @@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset + bs;
> region.dy = 0;
> region.height = info->var.yres_virtual;
> diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
> index ce08251bfd38..fd098ff17574 100644
> --- a/drivers/video/fbdev/core/fbcon_cw.c
> +++ b/drivers/video/fbdev/core/fbcon_cw.c
> @@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = 0;
> region.dy = info->var.yoffset + rs;
> region.height = rw;
> @@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dx = info->var.xoffset;
> region.dy = info->var.yoffset;
> region.height = info->var.yres;
> diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
> index 1936afc78fec..e165a3fad29a 100644
> --- a/drivers/video/fbdev/core/fbcon_ud.c
> +++ b/drivers/video/fbdev/core/fbcon_ud.c
> @@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dy = 0;
> region.dx = info->var.xoffset;
> region.width = rw;
> @@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
> info->fbops->fb_fillrect(info, ®ion);
> }
>
> - if (bh) {
> + if ((int) bh > 0) {
> region.dy = info->var.yoffset;
> region.dx = info->var.xoffset;
> region.height = bh;
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-14 7:22 ` Bartlomiej Zolnierkiewicz
(?)
@ 2020-07-14 10:27 ` Tetsuo Handa
-1 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-14 10:27 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
George Kennedy, Dan Carpenter, Jiri Slaby, Dmitry Vyukov
On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
> How does this patch relate to:
>
> https://marc.info/?l=linux-fbdev&m\x159415024816722&w=2
>
> ?
>
> It seems to address the same issue, I've added George and Dan to Cc:.
George Kennedy's patch does not help for my case.
You can try a.out built from
----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/fb.h>
int main(int argc, char *argv[])
{
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 16;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
return 0;
}
----------
with a fault injection patch
----------
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1214,6 +1214,10 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
if (new_screen_size > KMALLOC_MAX_SIZE)
return -EINVAL;
+ if (!strcmp(current->comm, "a.out")) {
+ printk(KERN_INFO "Forcing memory allocation failure.\n");
+ return -ENOMEM;
+ }
newscreen = kzalloc(new_screen_size, GFP_USER);
if (!newscreen)
return -ENOMEM;
----------
. What my patch workarounds is cases when vc_do_resize() did not update vc->vc_{cols,rows} .
Unless vc->vc_{cols,rows} are updated by vc_do_resize() in a way that avoids integer underflow at
unsigned int rw = info->var.xres - (vc->vc_cols*cw);
unsigned int bh = info->var.yres - (vc->vc_rows*ch);
, this crash won't go away.
[ 39.995757][ T2788] Forcing memory allocation failure.
[ 39.996527][ T2788] BUG: unable to handle page fault for address: ffffa9d180d7b000
[ 39.996529][ T2788] #PF: supervisor write access in kernel mode
[ 39.996530][ T2788] #PF: error_code(0x0002) - not-present page
[ 39.996531][ T2788] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 1324e4067 PTE 0
[ 39.996547][ T2788] Oops: 0002 [#1] SMP
[ 39.996550][ T2788] CPU: 2 PID: 2788 Comm: a.out Not tainted 5.8.0-rc5+ #757
[ 39.996551][ T2788] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[ 39.996555][ T2788] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-14 10:27 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-14 10:27 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-fbdev, dri-devel,
Dmitry Vyukov, linux-kernel, George Kennedy, Dan Carpenter
On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
> How does this patch relate to:
>
> https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
>
> ?
>
> It seems to address the same issue, I've added George and Dan to Cc:.
George Kennedy's patch does not help for my case.
You can try a.out built from
----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/fb.h>
int main(int argc, char *argv[])
{
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 16;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
return 0;
}
----------
with a fault injection patch
----------
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1214,6 +1214,10 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
if (new_screen_size > KMALLOC_MAX_SIZE)
return -EINVAL;
+ if (!strcmp(current->comm, "a.out")) {
+ printk(KERN_INFO "Forcing memory allocation failure.\n");
+ return -ENOMEM;
+ }
newscreen = kzalloc(new_screen_size, GFP_USER);
if (!newscreen)
return -ENOMEM;
----------
. What my patch workarounds is cases when vc_do_resize() did not update vc->vc_{cols,rows} .
Unless vc->vc_{cols,rows} are updated by vc_do_resize() in a way that avoids integer underflow at
unsigned int rw = info->var.xres - (vc->vc_cols*cw);
unsigned int bh = info->var.yres - (vc->vc_rows*ch);
, this crash won't go away.
[ 39.995757][ T2788] Forcing memory allocation failure.
[ 39.996527][ T2788] BUG: unable to handle page fault for address: ffffa9d180d7b000
[ 39.996529][ T2788] #PF: supervisor write access in kernel mode
[ 39.996530][ T2788] #PF: error_code(0x0002) - not-present page
[ 39.996531][ T2788] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 1324e4067 PTE 0
[ 39.996547][ T2788] Oops: 0002 [#1] SMP
[ 39.996550][ T2788] CPU: 2 PID: 2788 Comm: a.out Not tainted 5.8.0-rc5+ #757
[ 39.996551][ T2788] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[ 39.996555][ T2788] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-14 10:27 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-14 10:27 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
George Kennedy, Dan Carpenter, Jiri Slaby, Dmitry Vyukov
On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
> How does this patch relate to:
>
> https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
>
> ?
>
> It seems to address the same issue, I've added George and Dan to Cc:.
George Kennedy's patch does not help for my case.
You can try a.out built from
----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/fb.h>
int main(int argc, char *argv[])
{
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 16;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
return 0;
}
----------
with a fault injection patch
----------
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1214,6 +1214,10 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
if (new_screen_size > KMALLOC_MAX_SIZE)
return -EINVAL;
+ if (!strcmp(current->comm, "a.out")) {
+ printk(KERN_INFO "Forcing memory allocation failure.\n");
+ return -ENOMEM;
+ }
newscreen = kzalloc(new_screen_size, GFP_USER);
if (!newscreen)
return -ENOMEM;
----------
. What my patch workarounds is cases when vc_do_resize() did not update vc->vc_{cols,rows} .
Unless vc->vc_{cols,rows} are updated by vc_do_resize() in a way that avoids integer underflow at
unsigned int rw = info->var.xres - (vc->vc_cols*cw);
unsigned int bh = info->var.yres - (vc->vc_rows*ch);
, this crash won't go away.
[ 39.995757][ T2788] Forcing memory allocation failure.
[ 39.996527][ T2788] BUG: unable to handle page fault for address: ffffa9d180d7b000
[ 39.996529][ T2788] #PF: supervisor write access in kernel mode
[ 39.996530][ T2788] #PF: error_code(0x0002) - not-present page
[ 39.996531][ T2788] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 1324e4067 PTE 0
[ 39.996547][ T2788] Oops: 0002 [#1] SMP
[ 39.996550][ T2788] CPU: 2 PID: 2788 Comm: a.out Not tainted 5.8.0-rc5+ #757
[ 39.996551][ T2788] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[ 39.996555][ T2788] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-14 10:27 ` Tetsuo Handa
(?)
@ 2020-07-14 13:37 ` Tetsuo Handa
-1 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-14 13:37 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
George Kennedy, Dan Carpenter, Jiri Slaby, Dmitry Vyukov
On 2020/07/14 19:27, Tetsuo Handa wrote:
> On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
>> How does this patch relate to:
>>
>> https://marc.info/?l=linux-fbdev&m\x159415024816722&w=2
>>
>> ?
>>
>> It seems to address the same issue, I've added George and Dan to Cc:.
>
> George Kennedy's patch does not help for my case.
>
OK. You can add
Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
to my patch.
By the way, if
/* bitfill_aligned() assumes that it's at least 8x8 */
is true, don't we need to also check that the rect to fill is at least
8x8 in bit_clear_margins() ? (Well, I feel did it mean multiple of 8x8 ?
Then, what is bitfill_unaligned() for ?)
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-14 13:37 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-14 13:37 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-fbdev, dri-devel,
Dmitry Vyukov, linux-kernel, George Kennedy, Dan Carpenter
On 2020/07/14 19:27, Tetsuo Handa wrote:
> On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
>> How does this patch relate to:
>>
>> https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
>>
>> ?
>>
>> It seems to address the same issue, I've added George and Dan to Cc:.
>
> George Kennedy's patch does not help for my case.
>
OK. You can add
Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
to my patch.
By the way, if
/* bitfill_aligned() assumes that it's at least 8x8 */
is true, don't we need to also check that the rect to fill is at least
8x8 in bit_clear_margins() ? (Well, I feel did it mean multiple of 8x8 ?
Then, what is bitfill_unaligned() for ?)
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-14 13:37 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-14 13:37 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
George Kennedy, Dan Carpenter, Jiri Slaby, Dmitry Vyukov
On 2020/07/14 19:27, Tetsuo Handa wrote:
> On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
>> How does this patch relate to:
>>
>> https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
>>
>> ?
>>
>> It seems to address the same issue, I've added George and Dan to Cc:.
>
> George Kennedy's patch does not help for my case.
>
OK. You can add
Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
to my patch.
By the way, if
/* bitfill_aligned() assumes that it's at least 8x8 */
is true, don't we need to also check that the rect to fill is at least
8x8 in bit_clear_margins() ? (Well, I feel did it mean multiple of 8x8 ?
Then, what is bitfill_unaligned() for ?)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-14 13:37 ` Tetsuo Handa
(?)
@ 2020-07-15 1:51 ` Tetsuo Handa
-1 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 1:51 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Tetsuo Handa, Greg Kroah-Hartman, syzbot,
linux-kernel, dri-devel, George Kennedy, Dmitry Vyukov,
Jiri Slaby, Dan Carpenter
syzbot is reporting general protection fault in bitfill_aligned() [1]
caused by integer underflow in bit_clear_margins(). The cause of this
problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
try to overrun the __iomem region and causes general protection fault.
Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception. Since cols and lines are calculated as
cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
cols /= vc->vc_font.width;
rows /= vc->vc_font.height;
vc_resize(vc, cols, rows);
in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
and var.yres < vc->vc_font.height makes rows = 0. This means that
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 1;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
easily reproduces integer underflow bug explained above.
Of course, callers of vc_resize() are not handling vc_do_resize() failure
is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
as a band-aid workaround, this patch checks integer underflow in
"struct fbcon_ops"->clear_margins call, assuming that
vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
cause integer overflow.
[1] https://syzkaller.appspot.com/bug?id¥65882df74fa76f10d3a6fec4be31098dbb37c6
Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/video/fbdev/core/bitblit.c | 4 ++--
drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..35ebeeccde4d 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
region.width = rw;
@@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset + bs;
region.width = rs;
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index dfa9a8aa4509..78f3a5621478 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset;
region.height = rw;
@@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset + bs;
region.dy = 0;
region.height = info->var.yres_virtual;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index ce08251bfd38..fd098ff17574 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset + rs;
region.height = rw;
@@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset;
region.height = info->var.yres;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 1936afc78fec..e165a3fad29a 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dy = 0;
region.dx = info->var.xoffset;
region.width = rw;
@@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dy = info->var.yoffset;
region.dx = info->var.xoffset;
region.height = bh;
--
2.18.4
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 1:51 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 1:51 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Greg Kroah-Hartman, Jiri Slaby, Dmitry Vyukov, George Kennedy,
Dan Carpenter, dri-devel, linux-fbdev, linux-kernel, Tetsuo Handa,
syzbot
syzbot is reporting general protection fault in bitfill_aligned() [1]
caused by integer underflow in bit_clear_margins(). The cause of this
problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
try to overrun the __iomem region and causes general protection fault.
Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception. Since cols and lines are calculated as
cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
cols /= vc->vc_font.width;
rows /= vc->vc_font.height;
vc_resize(vc, cols, rows);
in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
and var.yres < vc->vc_font.height makes rows = 0. This means that
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 1;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
easily reproduces integer underflow bug explained above.
Of course, callers of vc_resize() are not handling vc_do_resize() failure
is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
as a band-aid workaround, this patch checks integer underflow in
"struct fbcon_ops"->clear_margins call, assuming that
vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
cause integer overflow.
[1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/video/fbdev/core/bitblit.c | 4 ++--
drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..35ebeeccde4d 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
region.width = rw;
@@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset + bs;
region.width = rs;
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index dfa9a8aa4509..78f3a5621478 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset;
region.height = rw;
@@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset + bs;
region.dy = 0;
region.height = info->var.yres_virtual;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index ce08251bfd38..fd098ff17574 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset + rs;
region.height = rw;
@@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset;
region.height = info->var.yres;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 1936afc78fec..e165a3fad29a 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dy = 0;
region.dx = info->var.xoffset;
region.width = rw;
@@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dy = info->var.yoffset;
region.dx = info->var.xoffset;
region.height = bh;
--
2.18.4
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 1:51 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 1:51 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Tetsuo Handa, Greg Kroah-Hartman, syzbot,
linux-kernel, dri-devel, George Kennedy, Dmitry Vyukov,
Jiri Slaby, Dan Carpenter
syzbot is reporting general protection fault in bitfill_aligned() [1]
caused by integer underflow in bit_clear_margins(). The cause of this
problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
try to overrun the __iomem region and causes general protection fault.
Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception. Since cols and lines are calculated as
cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
cols /= vc->vc_font.width;
rows /= vc->vc_font.height;
vc_resize(vc, cols, rows);
in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
and var.yres < vc->vc_font.height makes rows = 0. This means that
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 1;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
easily reproduces integer underflow bug explained above.
Of course, callers of vc_resize() are not handling vc_do_resize() failure
is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
as a band-aid workaround, this patch checks integer underflow in
"struct fbcon_ops"->clear_margins call, assuming that
vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
cause integer overflow.
[1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/video/fbdev/core/bitblit.c | 4 ++--
drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..35ebeeccde4d 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
region.width = rw;
@@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset + bs;
region.width = rs;
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index dfa9a8aa4509..78f3a5621478 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset;
region.height = rw;
@@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset + bs;
region.dy = 0;
region.height = info->var.yres_virtual;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index ce08251bfd38..fd098ff17574 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset + rs;
region.height = rw;
@@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset;
region.height = info->var.yres;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 1936afc78fec..e165a3fad29a 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -231,7 +231,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dy = 0;
region.dx = info->var.xoffset;
region.width = rw;
@@ -239,7 +239,7 @@ static void ud_clear_margins(struct vc_data *vc, struct fb_info *info,
info->fbops->fb_fillrect(info, ®ion);
}
- if (bh) {
+ if ((int) bh > 0) {
region.dy = info->var.yoffset;
region.dx = info->var.xoffset;
region.height = bh;
--
2.18.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-15 1:51 ` Tetsuo Handa
(?)
@ 2020-07-15 9:48 ` Dan Carpenter
-1 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-07-15 9:48 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote:
> syzbot is reporting general protection fault in bitfill_aligned() [1]
> caused by integer underflow in bit_clear_margins(). The cause of this
> problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
>
> If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
> is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
> bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
> info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
> try to overrun the __iomem region and causes general protection fault.
>
> Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception. Since cols and lines are calculated as
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
>
> in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
> and var.yres < vc->vc_font.height makes rows = 0. This means that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> easily reproduces integer underflow bug explained above.
>
> Of course, callers of vc_resize() are not handling vc_do_resize() failure
> is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
> as a band-aid workaround, this patch checks integer underflow in
> "struct fbcon_ops"->clear_margins call, assuming that
> vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
> cause integer overflow.
>
> [1] https://syzkaller.appspot.com/bug?id¥65882df74fa76f10d3a6fec4be31098dbb37c6
>
> Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
^^^^^^^^^^^^^^^^^^^^^^
If you choose a very high positive "rw" then this addition can overflow.
info->var.xoffset comes from the user and I don't think it's checked...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 9:48 ` Dan Carpenter
0 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-07-15 9:48 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jiri Slaby,
Dmitry Vyukov, George Kennedy, dri-devel, linux-fbdev,
linux-kernel, syzbot
On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote:
> syzbot is reporting general protection fault in bitfill_aligned() [1]
> caused by integer underflow in bit_clear_margins(). The cause of this
> problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
>
> If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
> is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
> bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
> info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
> try to overrun the __iomem region and causes general protection fault.
>
> Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception. Since cols and lines are calculated as
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
>
> in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
> and var.yres < vc->vc_font.height makes rows = 0. This means that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> easily reproduces integer underflow bug explained above.
>
> Of course, callers of vc_resize() are not handling vc_do_resize() failure
> is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
> as a band-aid workaround, this patch checks integer underflow in
> "struct fbcon_ops"->clear_margins call, assuming that
> vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
> cause integer overflow.
>
> [1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
>
> Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
^^^^^^^^^^^^^^^^^^^^^^
If you choose a very high positive "rw" then this addition can overflow.
info->var.xoffset comes from the user and I don't think it's checked...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 9:48 ` Dan Carpenter
0 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-07-15 9:48 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote:
> syzbot is reporting general protection fault in bitfill_aligned() [1]
> caused by integer underflow in bit_clear_margins(). The cause of this
> problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
>
> If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
> is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
> bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
> info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
> try to overrun the __iomem region and causes general protection fault.
>
> Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception. Since cols and lines are calculated as
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
>
> in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
> and var.yres < vc->vc_font.height makes rows = 0. This means that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> easily reproduces integer underflow bug explained above.
>
> Of course, callers of vc_resize() are not handling vc_do_resize() failure
> is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
> as a band-aid workaround, this patch checks integer underflow in
> "struct fbcon_ops"->clear_margins call, assuming that
> vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
> cause integer overflow.
>
> [1] https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
>
> Reported-and-tested-by: syzbot <syzbot+e5fd3e65515b48c02a30@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
^^^^^^^^^^^^^^^^^^^^^^
If you choose a very high positive "rw" then this addition can overflow.
info->var.xoffset comes from the user and I don't think it's checked...
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-15 9:48 ` Dan Carpenter
(?)
@ 2020-07-15 11:17 ` Tetsuo Handa
-1 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 11:17 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On 2020/07/15 18:48, Dan Carpenter wrote:
>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
>> region.color = color;
>> region.rop = ROP_COPY;
>>
>> - if (rw && !bottom_only) {
>> + if ((int) rw > 0 && !bottom_only) {
>> region.dx = info->var.xoffset + rs;
> ^^^^^^^^^^^^^^^^^^^^^^
>
> If you choose a very high positive "rw" then this addition can overflow.
> info->var.xoffset comes from the user and I don't think it's checked...
Well, I think it would be checked by "struct fb_ops"->check_var hook.
For example, vmw_fb_check_var() has
if ((var->xoffset + var->xres) > par->max_width ||
(var->yoffset + var->yres) > par->max_height) {
DRM_ERROR("Requested geom can not fit in framebuffer\n");
return -EINVAL;
}
check. Of course, there might be integer overflow in that check...
Having sanity check at caller of "struct fb_ops"->check_var might be nice.
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 11:17 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 11:17 UTC (permalink / raw)
To: Dan Carpenter
Cc: Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jiri Slaby,
Dmitry Vyukov, George Kennedy, dri-devel, linux-fbdev,
linux-kernel, syzbot
On 2020/07/15 18:48, Dan Carpenter wrote:
>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
>> region.color = color;
>> region.rop = ROP_COPY;
>>
>> - if (rw && !bottom_only) {
>> + if ((int) rw > 0 && !bottom_only) {
>> region.dx = info->var.xoffset + rs;
> ^^^^^^^^^^^^^^^^^^^^^^
>
> If you choose a very high positive "rw" then this addition can overflow.
> info->var.xoffset comes from the user and I don't think it's checked...
Well, I think it would be checked by "struct fb_ops"->check_var hook.
For example, vmw_fb_check_var() has
if ((var->xoffset + var->xres) > par->max_width ||
(var->yoffset + var->yres) > par->max_height) {
DRM_ERROR("Requested geom can not fit in framebuffer\n");
return -EINVAL;
}
check. Of course, there might be integer overflow in that check...
Having sanity check at caller of "struct fb_ops"->check_var might be nice.
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 11:17 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 11:17 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On 2020/07/15 18:48, Dan Carpenter wrote:
>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
>> region.color = color;
>> region.rop = ROP_COPY;
>>
>> - if (rw && !bottom_only) {
>> + if ((int) rw > 0 && !bottom_only) {
>> region.dx = info->var.xoffset + rs;
> ^^^^^^^^^^^^^^^^^^^^^^
>
> If you choose a very high positive "rw" then this addition can overflow.
> info->var.xoffset comes from the user and I don't think it's checked...
Well, I think it would be checked by "struct fb_ops"->check_var hook.
For example, vmw_fb_check_var() has
if ((var->xoffset + var->xres) > par->max_width ||
(var->yoffset + var->yres) > par->max_height) {
DRM_ERROR("Requested geom can not fit in framebuffer\n");
return -EINVAL;
}
check. Of course, there might be integer overflow in that check...
Having sanity check at caller of "struct fb_ops"->check_var might be nice.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-15 11:17 ` Tetsuo Handa
(?)
@ 2020-07-15 14:02 ` Tetsuo Handa
-1 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 14:02 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On 2020/07/15 20:17, Tetsuo Handa wrote:
> On 2020/07/15 18:48, Dan Carpenter wrote:
>>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
>>> region.color = color;
>>> region.rop = ROP_COPY;
>>>
>>> - if (rw && !bottom_only) {
>>> + if ((int) rw > 0 && !bottom_only) {
>>> region.dx = info->var.xoffset + rs;
>> ^^^^^^^^^^^^^^^^^^^^^^
>>
>> If you choose a very high positive "rw" then this addition can overflow.
>> info->var.xoffset comes from the user and I don't think it's checked...
>
> Well, I think it would be checked by "struct fb_ops"->check_var hook.
> For example, vmw_fb_check_var() has
>
> if ((var->xoffset + var->xres) > par->max_width ||
> (var->yoffset + var->yres) > par->max_height) {
> DRM_ERROR("Requested geom can not fit in framebuffer\n");
> return -EINVAL;
> }
>
> check. Of course, there might be integer overflow in that check...
> Having sanity check at caller of "struct fb_ops"->check_var might be nice.
>
Well, while
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 4;
var.xoffset = 4294967292U;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
bypassed
(var->xoffset + var->xres) > par->max_width
check in vmw_fb_check_var(),
----------
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
+ printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
----------
says that info->var.xoffset does not come from the user.
----------
bit_clear_margins info->var.xoffset=0 rs\x1024 info->var.yoffset=0 bs€0
----------
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 14:02 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 14:02 UTC (permalink / raw)
To: Dan Carpenter
Cc: Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jiri Slaby,
Dmitry Vyukov, George Kennedy, dri-devel, linux-fbdev,
linux-kernel, syzbot
On 2020/07/15 20:17, Tetsuo Handa wrote:
> On 2020/07/15 18:48, Dan Carpenter wrote:
>>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
>>> region.color = color;
>>> region.rop = ROP_COPY;
>>>
>>> - if (rw && !bottom_only) {
>>> + if ((int) rw > 0 && !bottom_only) {
>>> region.dx = info->var.xoffset + rs;
>> ^^^^^^^^^^^^^^^^^^^^^^
>>
>> If you choose a very high positive "rw" then this addition can overflow.
>> info->var.xoffset comes from the user and I don't think it's checked...
>
> Well, I think it would be checked by "struct fb_ops"->check_var hook.
> For example, vmw_fb_check_var() has
>
> if ((var->xoffset + var->xres) > par->max_width ||
> (var->yoffset + var->yres) > par->max_height) {
> DRM_ERROR("Requested geom can not fit in framebuffer\n");
> return -EINVAL;
> }
>
> check. Of course, there might be integer overflow in that check...
> Having sanity check at caller of "struct fb_ops"->check_var might be nice.
>
Well, while
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 4;
var.xoffset = 4294967292U;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
bypassed
(var->xoffset + var->xres) > par->max_width
check in vmw_fb_check_var(),
----------
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
+ printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
----------
says that info->var.xoffset does not come from the user.
----------
bit_clear_margins info->var.xoffset=0 rs=1024 info->var.yoffset=0 bs=800
----------
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 14:02 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 14:02 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On 2020/07/15 20:17, Tetsuo Handa wrote:
> On 2020/07/15 18:48, Dan Carpenter wrote:
>>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
>>> region.color = color;
>>> region.rop = ROP_COPY;
>>>
>>> - if (rw && !bottom_only) {
>>> + if ((int) rw > 0 && !bottom_only) {
>>> region.dx = info->var.xoffset + rs;
>> ^^^^^^^^^^^^^^^^^^^^^^
>>
>> If you choose a very high positive "rw" then this addition can overflow.
>> info->var.xoffset comes from the user and I don't think it's checked...
>
> Well, I think it would be checked by "struct fb_ops"->check_var hook.
> For example, vmw_fb_check_var() has
>
> if ((var->xoffset + var->xres) > par->max_width ||
> (var->yoffset + var->yres) > par->max_height) {
> DRM_ERROR("Requested geom can not fit in framebuffer\n");
> return -EINVAL;
> }
>
> check. Of course, there might be integer overflow in that check...
> Having sanity check at caller of "struct fb_ops"->check_var might be nice.
>
Well, while
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, &var);
var.xres = var.yres = 4;
var.xoffset = 4294967292U;
ioctl(fd, FBIOPUT_VSCREENINFO, &var);
bypassed
(var->xoffset + var->xres) > par->max_width
check in vmw_fb_check_var(),
----------
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
region.color = color;
region.rop = ROP_COPY;
+ printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
----------
says that info->var.xoffset does not come from the user.
----------
bit_clear_margins info->var.xoffset=0 rs=1024 info->var.yoffset=0 bs=800
----------
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-15 14:02 ` Tetsuo Handa
(?)
@ 2020-07-15 15:12 ` Dan Carpenter
-1 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-07-15 15:12 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On Wed, Jul 15, 2020 at 11:02:58PM +0900, Tetsuo Handa wrote:
> On 2020/07/15 20:17, Tetsuo Handa wrote:
> > On 2020/07/15 18:48, Dan Carpenter wrote:
> >>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> >>> region.color = color;
> >>> region.rop = ROP_COPY;
> >>>
> >>> - if (rw && !bottom_only) {
> >>> + if ((int) rw > 0 && !bottom_only) {
> >>> region.dx = info->var.xoffset + rs;
> >> ^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> If you choose a very high positive "rw" then this addition can overflow.
> >> info->var.xoffset comes from the user and I don't think it's checked...
> >
> > Well, I think it would be checked by "struct fb_ops"->check_var hook.
> > For example, vmw_fb_check_var() has
> >
> > if ((var->xoffset + var->xres) > par->max_width ||
> > (var->yoffset + var->yres) > par->max_height) {
> > DRM_ERROR("Requested geom can not fit in framebuffer\n");
> > return -EINVAL;
> > }
> >
> > check. Of course, there might be integer overflow in that check...
> > Having sanity check at caller of "struct fb_ops"->check_var might be nice.
> >
>
> Well, while
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 4;
> var.xoffset = 4294967292U;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> bypassed
>
> (var->xoffset + var->xres) > par->max_width
>
> check in vmw_fb_check_var(),
>
> ----------
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> + printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
> if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> ----------
>
> says that info->var.xoffset does not come from the user.
>
> ----------
> bit_clear_margins info->var.xoffset=0 rs\x1024 info->var.yoffset=0 bs€0
> ----------
In fb_set_var() we do:
drivers/video/fbdev/core/fbmem.c
1055 ret = info->fbops->fb_check_var(var, info);
1056
1057 if (ret)
1058 return ret;
1059
1060 if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
1061 return 0;
1062
1063 if (!basic_checks(var))
1064 return -EINVAL;
1065
1066 if (info->fbops->fb_get_caps) {
1067 ret = fb_check_caps(info, var, var->activate);
1068
1069 if (ret)
1070 return ret;
1071 }
1072
1073 old_var = info->var;
1074 info->var = *var;
^^^^^^^^^^^^^^^^
This should set "info->var.offset".
1075
1076 if (info->fbops->fb_set_par) {
1077 ret = info->fbops->fb_set_par(info);
1078
1079 if (ret) {
1080 info->var = old_var;
1081 printk(KERN_WARNING "detected "
I've complained about integer overflows in fbdev for a long time...
What I'd like to see is something like the following maybe. I don't
know how to get the vc_data in fbmem.c so it doesn't include your checks
for negative.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index caf817bcb05c..5c74181fea5d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -934,6 +934,54 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
}
EXPORT_SYMBOL(fb_pan_display);
+static bool basic_checks(struct fb_var_screeninfo *var)
+{
+ unsigned int v_margins, h_margins;
+
+ /* I think var->height and var->width = UINT_MAX means something. */
+
+ if (var->xres > INT_MAX ||
+ var->yres > INT_MAX ||
+ var->xres_virtual > INT_MAX ||
+ var->yres_virtual > INT_MAX ||
+ var->xoffset > INT_MAX ||
+ var->yoffset > INT_MAX ||
+ var->left_margin > INT_MAX ||
+ var->right_margin > INT_MAX ||
+ var->upper_margin > INT_MAX ||
+ var->lower_margin > INT_MAX ||
+ var->hsync_len > INT_MAX ||
+ var->vsync_len > INT_MAX)
+ return false;
+
+ if (var->bits_per_pixel > 128)
+ return false;
+ if (var->rotate > FB_ROTATE_CCW)
+ return false;
+
+ if (var->xoffset > INT_MAX - var->xres)
+ return false;
+ if (var->yoffset > INT_MAX - var->yres)
+ return false;
+
+ if (var->left_margin > INT_MAX - var->right_margin ||
+ var->upper_margin > INT_MAX - var->lower_margin)
+ return false;
+
+ v_margins = var->left_margin + var->right_margin;
+ h_margins = var->upper_margin + var->lower_margin;
+
+ if (var->xres > INT_MAX - var->hsync_len ||
+ var->yres > INT_MAX - var->vsync_len)
+ return false;
+
+ if (v_margins > INT_MAX - var->hsync_len - var->xres ||
+ h_margins > INT_MAX - var->vsync_len - var->yres)
+ return false;
+
+ return true;
+}
+
static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
u32 activate)
{
@@ -1012,6 +1060,9 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
return 0;
+ if (!basic_checks(var))
+ return -EINVAL;
+
if (info->fbops->fb_get_caps) {
ret = fb_check_caps(info, var, var->activate);
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 15:12 ` Dan Carpenter
0 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-07-15 15:12 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jiri Slaby,
Dmitry Vyukov, George Kennedy, dri-devel, linux-fbdev,
linux-kernel, syzbot
On Wed, Jul 15, 2020 at 11:02:58PM +0900, Tetsuo Handa wrote:
> On 2020/07/15 20:17, Tetsuo Handa wrote:
> > On 2020/07/15 18:48, Dan Carpenter wrote:
> >>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> >>> region.color = color;
> >>> region.rop = ROP_COPY;
> >>>
> >>> - if (rw && !bottom_only) {
> >>> + if ((int) rw > 0 && !bottom_only) {
> >>> region.dx = info->var.xoffset + rs;
> >> ^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> If you choose a very high positive "rw" then this addition can overflow.
> >> info->var.xoffset comes from the user and I don't think it's checked...
> >
> > Well, I think it would be checked by "struct fb_ops"->check_var hook.
> > For example, vmw_fb_check_var() has
> >
> > if ((var->xoffset + var->xres) > par->max_width ||
> > (var->yoffset + var->yres) > par->max_height) {
> > DRM_ERROR("Requested geom can not fit in framebuffer\n");
> > return -EINVAL;
> > }
> >
> > check. Of course, there might be integer overflow in that check...
> > Having sanity check at caller of "struct fb_ops"->check_var might be nice.
> >
>
> Well, while
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 4;
> var.xoffset = 4294967292U;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> bypassed
>
> (var->xoffset + var->xres) > par->max_width
>
> check in vmw_fb_check_var(),
>
> ----------
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> + printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
> if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> ----------
>
> says that info->var.xoffset does not come from the user.
>
> ----------
> bit_clear_margins info->var.xoffset=0 rs=1024 info->var.yoffset=0 bs=800
> ----------
In fb_set_var() we do:
drivers/video/fbdev/core/fbmem.c
1055 ret = info->fbops->fb_check_var(var, info);
1056
1057 if (ret)
1058 return ret;
1059
1060 if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
1061 return 0;
1062
1063 if (!basic_checks(var))
1064 return -EINVAL;
1065
1066 if (info->fbops->fb_get_caps) {
1067 ret = fb_check_caps(info, var, var->activate);
1068
1069 if (ret)
1070 return ret;
1071 }
1072
1073 old_var = info->var;
1074 info->var = *var;
^^^^^^^^^^^^^^^^
This should set "info->var.offset".
1075
1076 if (info->fbops->fb_set_par) {
1077 ret = info->fbops->fb_set_par(info);
1078
1079 if (ret) {
1080 info->var = old_var;
1081 printk(KERN_WARNING "detected "
I've complained about integer overflows in fbdev for a long time...
What I'd like to see is something like the following maybe. I don't
know how to get the vc_data in fbmem.c so it doesn't include your checks
for negative.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index caf817bcb05c..5c74181fea5d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -934,6 +934,54 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
}
EXPORT_SYMBOL(fb_pan_display);
+static bool basic_checks(struct fb_var_screeninfo *var)
+{
+ unsigned int v_margins, h_margins;
+
+ /* I think var->height and var->width == UINT_MAX means something. */
+
+ if (var->xres > INT_MAX ||
+ var->yres > INT_MAX ||
+ var->xres_virtual > INT_MAX ||
+ var->yres_virtual > INT_MAX ||
+ var->xoffset > INT_MAX ||
+ var->yoffset > INT_MAX ||
+ var->left_margin > INT_MAX ||
+ var->right_margin > INT_MAX ||
+ var->upper_margin > INT_MAX ||
+ var->lower_margin > INT_MAX ||
+ var->hsync_len > INT_MAX ||
+ var->vsync_len > INT_MAX)
+ return false;
+
+ if (var->bits_per_pixel > 128)
+ return false;
+ if (var->rotate > FB_ROTATE_CCW)
+ return false;
+
+ if (var->xoffset > INT_MAX - var->xres)
+ return false;
+ if (var->yoffset > INT_MAX - var->yres)
+ return false;
+
+ if (var->left_margin > INT_MAX - var->right_margin ||
+ var->upper_margin > INT_MAX - var->lower_margin)
+ return false;
+
+ v_margins = var->left_margin + var->right_margin;
+ h_margins = var->upper_margin + var->lower_margin;
+
+ if (var->xres > INT_MAX - var->hsync_len ||
+ var->yres > INT_MAX - var->vsync_len)
+ return false;
+
+ if (v_margins > INT_MAX - var->hsync_len - var->xres ||
+ h_margins > INT_MAX - var->vsync_len - var->yres)
+ return false;
+
+ return true;
+}
+
static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
u32 activate)
{
@@ -1012,6 +1060,9 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
return 0;
+ if (!basic_checks(var))
+ return -EINVAL;
+
if (info->fbops->fb_get_caps) {
ret = fb_check_caps(info, var, var->activate);
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 15:12 ` Dan Carpenter
0 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-07-15 15:12 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On Wed, Jul 15, 2020 at 11:02:58PM +0900, Tetsuo Handa wrote:
> On 2020/07/15 20:17, Tetsuo Handa wrote:
> > On 2020/07/15 18:48, Dan Carpenter wrote:
> >>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> >>> region.color = color;
> >>> region.rop = ROP_COPY;
> >>>
> >>> - if (rw && !bottom_only) {
> >>> + if ((int) rw > 0 && !bottom_only) {
> >>> region.dx = info->var.xoffset + rs;
> >> ^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> If you choose a very high positive "rw" then this addition can overflow.
> >> info->var.xoffset comes from the user and I don't think it's checked...
> >
> > Well, I think it would be checked by "struct fb_ops"->check_var hook.
> > For example, vmw_fb_check_var() has
> >
> > if ((var->xoffset + var->xres) > par->max_width ||
> > (var->yoffset + var->yres) > par->max_height) {
> > DRM_ERROR("Requested geom can not fit in framebuffer\n");
> > return -EINVAL;
> > }
> >
> > check. Of course, there might be integer overflow in that check...
> > Having sanity check at caller of "struct fb_ops"->check_var might be nice.
> >
>
> Well, while
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 4;
> var.xoffset = 4294967292U;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
>
> bypassed
>
> (var->xoffset + var->xres) > par->max_width
>
> check in vmw_fb_check_var(),
>
> ----------
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> + printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
> if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> ----------
>
> says that info->var.xoffset does not come from the user.
>
> ----------
> bit_clear_margins info->var.xoffset=0 rs=1024 info->var.yoffset=0 bs=800
> ----------
In fb_set_var() we do:
drivers/video/fbdev/core/fbmem.c
1055 ret = info->fbops->fb_check_var(var, info);
1056
1057 if (ret)
1058 return ret;
1059
1060 if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
1061 return 0;
1062
1063 if (!basic_checks(var))
1064 return -EINVAL;
1065
1066 if (info->fbops->fb_get_caps) {
1067 ret = fb_check_caps(info, var, var->activate);
1068
1069 if (ret)
1070 return ret;
1071 }
1072
1073 old_var = info->var;
1074 info->var = *var;
^^^^^^^^^^^^^^^^
This should set "info->var.offset".
1075
1076 if (info->fbops->fb_set_par) {
1077 ret = info->fbops->fb_set_par(info);
1078
1079 if (ret) {
1080 info->var = old_var;
1081 printk(KERN_WARNING "detected "
I've complained about integer overflows in fbdev for a long time...
What I'd like to see is something like the following maybe. I don't
know how to get the vc_data in fbmem.c so it doesn't include your checks
for negative.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index caf817bcb05c..5c74181fea5d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -934,6 +934,54 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
}
EXPORT_SYMBOL(fb_pan_display);
+static bool basic_checks(struct fb_var_screeninfo *var)
+{
+ unsigned int v_margins, h_margins;
+
+ /* I think var->height and var->width == UINT_MAX means something. */
+
+ if (var->xres > INT_MAX ||
+ var->yres > INT_MAX ||
+ var->xres_virtual > INT_MAX ||
+ var->yres_virtual > INT_MAX ||
+ var->xoffset > INT_MAX ||
+ var->yoffset > INT_MAX ||
+ var->left_margin > INT_MAX ||
+ var->right_margin > INT_MAX ||
+ var->upper_margin > INT_MAX ||
+ var->lower_margin > INT_MAX ||
+ var->hsync_len > INT_MAX ||
+ var->vsync_len > INT_MAX)
+ return false;
+
+ if (var->bits_per_pixel > 128)
+ return false;
+ if (var->rotate > FB_ROTATE_CCW)
+ return false;
+
+ if (var->xoffset > INT_MAX - var->xres)
+ return false;
+ if (var->yoffset > INT_MAX - var->yres)
+ return false;
+
+ if (var->left_margin > INT_MAX - var->right_margin ||
+ var->upper_margin > INT_MAX - var->lower_margin)
+ return false;
+
+ v_margins = var->left_margin + var->right_margin;
+ h_margins = var->upper_margin + var->lower_margin;
+
+ if (var->xres > INT_MAX - var->hsync_len ||
+ var->yres > INT_MAX - var->vsync_len)
+ return false;
+
+ if (v_margins > INT_MAX - var->hsync_len - var->xres ||
+ h_margins > INT_MAX - var->vsync_len - var->yres)
+ return false;
+
+ return true;
+}
+
static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
u32 activate)
{
@@ -1012,6 +1060,9 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
return 0;
+ if (!basic_checks(var))
+ return -EINVAL;
+
if (info->fbops->fb_get_caps) {
ret = fb_check_caps(info, var, var->activate);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-15 15:12 ` Dan Carpenter
(?)
@ 2020-07-15 15:29 ` Tetsuo Handa
-1 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 15:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On 2020/07/16 0:12, Dan Carpenter wrote:
> I've complained about integer overflows in fbdev for a long time...
>
> What I'd like to see is something like the following maybe. I don't
> know how to get the vc_data in fbmem.c so it doesn't include your checks
> for negative.
Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
we want basic checks. That's a task for fbdev people who should be familiar with
necessary constraints.
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 15:29 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 15:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jiri Slaby,
Dmitry Vyukov, George Kennedy, dri-devel, linux-fbdev,
linux-kernel, syzbot
On 2020/07/16 0:12, Dan Carpenter wrote:
> I've complained about integer overflows in fbdev for a long time...
>
> What I'd like to see is something like the following maybe. I don't
> know how to get the vc_data in fbmem.c so it doesn't include your checks
> for negative.
Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
we want basic checks. That's a task for fbdev people who should be familiar with
necessary constraints.
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 15:29 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 15:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On 2020/07/16 0:12, Dan Carpenter wrote:
> I've complained about integer overflows in fbdev for a long time...
>
> What I'd like to see is something like the following maybe. I don't
> know how to get the vc_data in fbmem.c so it doesn't include your checks
> for negative.
Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
we want basic checks. That's a task for fbdev people who should be familiar with
necessary constraints.
Anyway, my two patches are small and low cost; can we apply these patches regardless
of basic checks?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-15 15:29 ` Tetsuo Handa
(?)
@ 2020-07-16 10:00 ` Daniel Vetter
-1 siblings, 0 replies; 77+ messages in thread
From: Daniel Vetter @ 2020-07-16 10:00 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Dmitry Vyukov,
Jiri Slaby, Dan Carpenter
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> On 2020/07/16 0:12, Dan Carpenter wrote:
> > I've complained about integer overflows in fbdev for a long time...
> >
> > What I'd like to see is something like the following maybe. I don't
> > know how to get the vc_data in fbmem.c so it doesn't include your checks
> > for negative.
>
> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> we want basic checks. That's a task for fbdev people who should be familiar with
> necessary constraints.
I think the worldwide supply of people who understand fbdev and willing to
work on it is roughly 0. So if someone wants to fix this mess properly
(which likely means adding tons of over/underflow checks at entry points,
since you're never going to catch the driver bugs, there's too many and
not enough people who care) they need to fix this themselves.
Just to avoid confusion here.
> Anyway, my two patches are small and low cost; can we apply these patches regardless
> of basic checks?
Which two patches where?
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-16 10:00 ` Daniel Vetter
0 siblings, 0 replies; 77+ messages in thread
From: Daniel Vetter @ 2020-07-16 10:00 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Dan Carpenter, linux-fbdev, Bartlomiej Zolnierkiewicz,
Greg Kroah-Hartman, syzbot, linux-kernel, dri-devel,
George Kennedy, Jiri Slaby, Dmitry Vyukov
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> On 2020/07/16 0:12, Dan Carpenter wrote:
> > I've complained about integer overflows in fbdev for a long time...
> >
> > What I'd like to see is something like the following maybe. I don't
> > know how to get the vc_data in fbmem.c so it doesn't include your checks
> > for negative.
>
> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> we want basic checks. That's a task for fbdev people who should be familiar with
> necessary constraints.
I think the worldwide supply of people who understand fbdev and willing to
work on it is roughly 0. So if someone wants to fix this mess properly
(which likely means adding tons of over/underflow checks at entry points,
since you're never going to catch the driver bugs, there's too many and
not enough people who care) they need to fix this themselves.
Just to avoid confusion here.
> Anyway, my two patches are small and low cost; can we apply these patches regardless
> of basic checks?
Which two patches where?
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-16 10:00 ` Daniel Vetter
0 siblings, 0 replies; 77+ messages in thread
From: Daniel Vetter @ 2020-07-16 10:00 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Dmitry Vyukov,
Jiri Slaby, Dan Carpenter
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> On 2020/07/16 0:12, Dan Carpenter wrote:
> > I've complained about integer overflows in fbdev for a long time...
> >
> > What I'd like to see is something like the following maybe. I don't
> > know how to get the vc_data in fbmem.c so it doesn't include your checks
> > for negative.
>
> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> we want basic checks. That's a task for fbdev people who should be familiar with
> necessary constraints.
I think the worldwide supply of people who understand fbdev and willing to
work on it is roughly 0. So if someone wants to fix this mess properly
(which likely means adding tons of over/underflow checks at entry points,
since you're never going to catch the driver bugs, there's too many and
not enough people who care) they need to fix this themselves.
Just to avoid confusion here.
> Anyway, my two patches are small and low cost; can we apply these patches regardless
> of basic checks?
Which two patches where?
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-16 10:00 ` Daniel Vetter
(?)
@ 2020-07-16 11:27 ` Tetsuo Handa
-1 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-16 11:27 UTC (permalink / raw)
To: Daniel Vetter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Dmitry Vyukov,
Jiri Slaby, Dan Carpenter
On 2020/07/16 19:00, Daniel Vetter wrote:
> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>> I've complained about integer overflows in fbdev for a long time...
>>>
>>> What I'd like to see is something like the following maybe. I don't
>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>> for negative.
>>
>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>> we want basic checks. That's a task for fbdev people who should be familiar with
>> necessary constraints.
>
> I think the worldwide supply of people who understand fbdev and willing to
> work on it is roughly 0. So if someone wants to fix this mess properly
> (which likely means adding tons of over/underflow checks at entry points,
> since you're never going to catch the driver bugs, there's too many and
> not enough people who care) they need to fix this themselves.
But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
(which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
"32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
rows and cols < 32768 ?
>
> Just to avoid confusion here.
>
>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>> of basic checks?
>
> Which two patches where?
[PATCH v3] vt: Reject zero-sized screen buffer size.
from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
[PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-16 11:27 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-16 11:27 UTC (permalink / raw)
To: Daniel Vetter
Cc: Dan Carpenter, linux-fbdev, Bartlomiej Zolnierkiewicz,
Greg Kroah-Hartman, syzbot, linux-kernel, dri-devel,
George Kennedy, Jiri Slaby, Dmitry Vyukov
On 2020/07/16 19:00, Daniel Vetter wrote:
> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>> I've complained about integer overflows in fbdev for a long time...
>>>
>>> What I'd like to see is something like the following maybe. I don't
>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>> for negative.
>>
>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>> we want basic checks. That's a task for fbdev people who should be familiar with
>> necessary constraints.
>
> I think the worldwide supply of people who understand fbdev and willing to
> work on it is roughly 0. So if someone wants to fix this mess properly
> (which likely means adding tons of over/underflow checks at entry points,
> since you're never going to catch the driver bugs, there's too many and
> not enough people who care) they need to fix this themselves.
But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
(which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
"32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
rows and cols < 32768 ?
>
> Just to avoid confusion here.
>
>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>> of basic checks?
>
> Which two patches where?
[PATCH v3] vt: Reject zero-sized screen buffer size.
from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
[PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-16 11:27 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-16 11:27 UTC (permalink / raw)
To: Daniel Vetter
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
syzbot, linux-kernel, dri-devel, George Kennedy, Dmitry Vyukov,
Jiri Slaby, Dan Carpenter
On 2020/07/16 19:00, Daniel Vetter wrote:
> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>> I've complained about integer overflows in fbdev for a long time...
>>>
>>> What I'd like to see is something like the following maybe. I don't
>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>> for negative.
>>
>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>> we want basic checks. That's a task for fbdev people who should be familiar with
>> necessary constraints.
>
> I think the worldwide supply of people who understand fbdev and willing to
> work on it is roughly 0. So if someone wants to fix this mess properly
> (which likely means adding tons of over/underflow checks at entry points,
> since you're never going to catch the driver bugs, there's too many and
> not enough people who care) they need to fix this themselves.
But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
(which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
"32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
rows and cols < 32768 ?
>
> Just to avoid confusion here.
>
>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>> of basic checks?
>
> Which two patches where?
[PATCH v3] vt: Reject zero-sized screen buffer size.
from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
[PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-16 11:27 ` Tetsuo Handa
(?)
@ 2020-07-21 16:08 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 77+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-21 16:08 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, syzbot, linux-kernel,
dri-devel, George Kennedy, Dmitry Vyukov, Jiri Slaby,
Dan Carpenter
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> On 2020/07/16 19:00, Daniel Vetter wrote:
> > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> >> On 2020/07/16 0:12, Dan Carpenter wrote:
> >>> I've complained about integer overflows in fbdev for a long time...
> >>>
> >>> What I'd like to see is something like the following maybe. I don't
> >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> >>> for negative.
> >>
> >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> >> we want basic checks. That's a task for fbdev people who should be familiar with
> >> necessary constraints.
> >
> > I think the worldwide supply of people who understand fbdev and willing to
> > work on it is roughly 0. So if someone wants to fix this mess properly
> > (which likely means adding tons of over/underflow checks at entry points,
> > since you're never going to catch the driver bugs, there's too many and
> > not enough people who care) they need to fix this themselves.
>
> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> rows and cols < 32768 ?
>
> >
> > Just to avoid confusion here.
> >
> >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> >> of basic checks?
> >
> > Which two patches where?
>
> [PATCH v3] vt: Reject zero-sized screen buffer size.
> from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
This is now in my tree.
> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
That should be taken by the fbdev maintainer, but I can take it too if
people want.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-21 16:08 ` Greg Kroah-Hartman
0 siblings, 0 replies; 77+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-21 16:08 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Daniel Vetter, Dan Carpenter, linux-fbdev,
Bartlomiej Zolnierkiewicz, syzbot, linux-kernel, dri-devel,
George Kennedy, Jiri Slaby, Dmitry Vyukov
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> On 2020/07/16 19:00, Daniel Vetter wrote:
> > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> >> On 2020/07/16 0:12, Dan Carpenter wrote:
> >>> I've complained about integer overflows in fbdev for a long time...
> >>>
> >>> What I'd like to see is something like the following maybe. I don't
> >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> >>> for negative.
> >>
> >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> >> we want basic checks. That's a task for fbdev people who should be familiar with
> >> necessary constraints.
> >
> > I think the worldwide supply of people who understand fbdev and willing to
> > work on it is roughly 0. So if someone wants to fix this mess properly
> > (which likely means adding tons of over/underflow checks at entry points,
> > since you're never going to catch the driver bugs, there's too many and
> > not enough people who care) they need to fix this themselves.
>
> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> rows and cols < 32768 ?
>
> >
> > Just to avoid confusion here.
> >
> >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> >> of basic checks?
> >
> > Which two patches where?
>
> [PATCH v3] vt: Reject zero-sized screen buffer size.
> from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
This is now in my tree.
> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
That should be taken by the fbdev maintainer, but I can take it too if
people want.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-21 16:08 ` Greg Kroah-Hartman
0 siblings, 0 replies; 77+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-21 16:08 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, syzbot, linux-kernel,
dri-devel, George Kennedy, Dmitry Vyukov, Jiri Slaby,
Dan Carpenter
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> On 2020/07/16 19:00, Daniel Vetter wrote:
> > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> >> On 2020/07/16 0:12, Dan Carpenter wrote:
> >>> I've complained about integer overflows in fbdev for a long time...
> >>>
> >>> What I'd like to see is something like the following maybe. I don't
> >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> >>> for negative.
> >>
> >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> >> we want basic checks. That's a task for fbdev people who should be familiar with
> >> necessary constraints.
> >
> > I think the worldwide supply of people who understand fbdev and willing to
> > work on it is roughly 0. So if someone wants to fix this mess properly
> > (which likely means adding tons of over/underflow checks at entry points,
> > since you're never going to catch the driver bugs, there's too many and
> > not enough people who care) they need to fix this themselves.
>
> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> rows and cols < 32768 ?
>
> >
> > Just to avoid confusion here.
> >
> >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> >> of basic checks?
> >
> > Which two patches where?
>
> [PATCH v3] vt: Reject zero-sized screen buffer size.
> from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
This is now in my tree.
> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
That should be taken by the fbdev maintainer, but I can take it too if
people want.
thanks,
greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-21 16:08 ` Greg Kroah-Hartman
(?)
@ 2020-07-22 8:07 ` Daniel Vetter
-1 siblings, 0 replies; 77+ messages in thread
From: Daniel Vetter @ 2020-07-22 8:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
Tetsuo Handa, syzbot, Linux Kernel Mailing List, dri-devel,
George Kennedy, Dmitry Vyukov, Jiri Slaby, Dan Carpenter
On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > >>> I've complained about integer overflows in fbdev for a long time...
> > >>>
> > >>> What I'd like to see is something like the following maybe. I don't
> > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > >>> for negative.
> > >>
> > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > >> necessary constraints.
> > >
> > > I think the worldwide supply of people who understand fbdev and willing to
> > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > (which likely means adding tons of over/underflow checks at entry points,
> > > since you're never going to catch the driver bugs, there's too many and
> > > not enough people who care) they need to fix this themselves.
> >
> > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > rows and cols < 32768 ?
> >
> > >
> > > Just to avoid confusion here.
> > >
> > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > >> of basic checks?
> > >
> > > Which two patches where?
> >
> > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> This is now in my tree.
>
> > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> That should be taken by the fbdev maintainer, but I can take it too if
> people want.
Just missed this weeks pull request train and feeling like not worth
making this an exception (it's been broken forever after all), so
maybe best if you just add this to vt.
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Also this avoids the impression I know what's going on in fbdev code,
maybe with sufficient abandon from my side someone will pop up who
cares an fixes the bazillion of syzkaller issues we seem to have
around console/vt and everything related.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-22 8:07 ` Daniel Vetter
0 siblings, 0 replies; 77+ messages in thread
From: Daniel Vetter @ 2020-07-22 8:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Tetsuo Handa, Dan Carpenter, Linux Fbdev development list,
Bartlomiej Zolnierkiewicz, syzbot, Linux Kernel Mailing List,
dri-devel, George Kennedy, Jiri Slaby, Dmitry Vyukov
On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > >>> I've complained about integer overflows in fbdev for a long time...
> > >>>
> > >>> What I'd like to see is something like the following maybe. I don't
> > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > >>> for negative.
> > >>
> > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > >> necessary constraints.
> > >
> > > I think the worldwide supply of people who understand fbdev and willing to
> > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > (which likely means adding tons of over/underflow checks at entry points,
> > > since you're never going to catch the driver bugs, there's too many and
> > > not enough people who care) they need to fix this themselves.
> >
> > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > rows and cols < 32768 ?
> >
> > >
> > > Just to avoid confusion here.
> > >
> > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > >> of basic checks?
> > >
> > > Which two patches where?
> >
> > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> This is now in my tree.
>
> > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> That should be taken by the fbdev maintainer, but I can take it too if
> people want.
Just missed this weeks pull request train and feeling like not worth
making this an exception (it's been broken forever after all), so
maybe best if you just add this to vt.
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Also this avoids the impression I know what's going on in fbdev code,
maybe with sufficient abandon from my side someone will pop up who
cares an fixes the bazillion of syzkaller issues we seem to have
around console/vt and everything related.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-22 8:07 ` Daniel Vetter
0 siblings, 0 replies; 77+ messages in thread
From: Daniel Vetter @ 2020-07-22 8:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
Tetsuo Handa, syzbot, Linux Kernel Mailing List, dri-devel,
George Kennedy, Dmitry Vyukov, Jiri Slaby, Dan Carpenter
On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > >>> I've complained about integer overflows in fbdev for a long time...
> > >>>
> > >>> What I'd like to see is something like the following maybe. I don't
> > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > >>> for negative.
> > >>
> > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > >> necessary constraints.
> > >
> > > I think the worldwide supply of people who understand fbdev and willing to
> > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > (which likely means adding tons of over/underflow checks at entry points,
> > > since you're never going to catch the driver bugs, there's too many and
> > > not enough people who care) they need to fix this themselves.
> >
> > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > rows and cols < 32768 ?
> >
> > >
> > > Just to avoid confusion here.
> > >
> > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > >> of basic checks?
> > >
> > > Which two patches where?
> >
> > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> This is now in my tree.
>
> > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>
> That should be taken by the fbdev maintainer, but I can take it too if
> people want.
Just missed this weeks pull request train and feeling like not worth
making this an exception (it's been broken forever after all), so
maybe best if you just add this to vt.
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Also this avoids the impression I know what's going on in fbdev code,
maybe with sufficient abandon from my side someone will pop up who
cares an fixes the bazillion of syzkaller issues we seem to have
around console/vt and everything related.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-22 8:07 ` Daniel Vetter
(?)
@ 2020-07-23 14:21 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 77+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-23 14:21 UTC (permalink / raw)
To: Daniel Vetter
Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
Tetsuo Handa, syzbot, Linux Kernel Mailing List, dri-devel,
George Kennedy, Dmitry Vyukov, Jiri Slaby, Dan Carpenter
On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > > >>> I've complained about integer overflows in fbdev for a long time...
> > > >>>
> > > >>> What I'd like to see is something like the following maybe. I don't
> > > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > > >>> for negative.
> > > >>
> > > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > > >> necessary constraints.
> > > >
> > > > I think the worldwide supply of people who understand fbdev and willing to
> > > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > > (which likely means adding tons of over/underflow checks at entry points,
> > > > since you're never going to catch the driver bugs, there's too many and
> > > > not enough people who care) they need to fix this themselves.
> > >
> > > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > > rows and cols < 32768 ?
> > >
> > > >
> > > > Just to avoid confusion here.
> > > >
> > > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > > >> of basic checks?
> > > >
> > > > Which two patches where?
> > >
> > > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > This is now in my tree.
> >
> > > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> > > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > That should be taken by the fbdev maintainer, but I can take it too if
> > people want.
>
> Just missed this weeks pull request train and feeling like not worth
> making this an exception (it's been broken forever after all), so
> maybe best if you just add this to vt.
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Also this avoids the impression I know what's going on in fbdev code,
> maybe with sufficient abandon from my side someone will pop up who
> cares an fixes the bazillion of syzkaller issues we seem to have
> around console/vt and everything related.
Great, will go queue it up now, thanks!
greg k-h
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-23 14:21 ` Greg Kroah-Hartman
0 siblings, 0 replies; 77+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-23 14:21 UTC (permalink / raw)
To: Daniel Vetter
Cc: Tetsuo Handa, Dan Carpenter, Linux Fbdev development list,
Bartlomiej Zolnierkiewicz, syzbot, Linux Kernel Mailing List,
dri-devel, George Kennedy, Jiri Slaby, Dmitry Vyukov
On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > > >>> I've complained about integer overflows in fbdev for a long time...
> > > >>>
> > > >>> What I'd like to see is something like the following maybe. I don't
> > > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > > >>> for negative.
> > > >>
> > > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > > >> necessary constraints.
> > > >
> > > > I think the worldwide supply of people who understand fbdev and willing to
> > > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > > (which likely means adding tons of over/underflow checks at entry points,
> > > > since you're never going to catch the driver bugs, there's too many and
> > > > not enough people who care) they need to fix this themselves.
> > >
> > > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > > rows and cols < 32768 ?
> > >
> > > >
> > > > Just to avoid confusion here.
> > > >
> > > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > > >> of basic checks?
> > > >
> > > > Which two patches where?
> > >
> > > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > This is now in my tree.
> >
> > > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> > > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > That should be taken by the fbdev maintainer, but I can take it too if
> > people want.
>
> Just missed this weeks pull request train and feeling like not worth
> making this an exception (it's been broken forever after all), so
> maybe best if you just add this to vt.
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Also this avoids the impression I know what's going on in fbdev code,
> maybe with sufficient abandon from my side someone will pop up who
> cares an fixes the bazillion of syzkaller issues we seem to have
> around console/vt and everything related.
Great, will go queue it up now, thanks!
greg k-h
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-23 14:21 ` Greg Kroah-Hartman
0 siblings, 0 replies; 77+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-23 14:21 UTC (permalink / raw)
To: Daniel Vetter
Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
Tetsuo Handa, syzbot, Linux Kernel Mailing List, dri-devel,
George Kennedy, Dmitry Vyukov, Jiri Slaby, Dan Carpenter
On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > > >>> I've complained about integer overflows in fbdev for a long time...
> > > >>>
> > > >>> What I'd like to see is something like the following maybe. I don't
> > > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > > >>> for negative.
> > > >>
> > > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
> > > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
> > > >> we want basic checks. That's a task for fbdev people who should be familiar with
> > > >> necessary constraints.
> > > >
> > > > I think the worldwide supply of people who understand fbdev and willing to
> > > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > > (which likely means adding tons of over/underflow checks at entry points,
> > > > since you're never going to catch the driver bugs, there's too many and
> > > > not enough people who care) they need to fix this themselves.
> > >
> > > But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
> > > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
> > > "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
> > > rows and cols < 32768 ?
> > >
> > > >
> > > > Just to avoid confusion here.
> > > >
> > > >> Anyway, my two patches are small and low cost; can we apply these patches regardless
> > > >> of basic checks?
> > > >
> > > > Which two patches where?
> > >
> > > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > > from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > This is now in my tree.
> >
> > > [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
> > > from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
> >
> > That should be taken by the fbdev maintainer, but I can take it too if
> > people want.
>
> Just missed this weeks pull request train and feeling like not worth
> making this an exception (it's been broken forever after all), so
> maybe best if you just add this to vt.
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Also this avoids the impression I know what's going on in fbdev code,
> maybe with sufficient abandon from my side someone will pop up who
> cares an fixes the bazillion of syzkaller issues we seem to have
> around console/vt and everything related.
Great, will go queue it up now, thanks!
greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-23 14:21 ` Greg Kroah-Hartman
(?)
@ 2020-07-24 8:28 ` Bartlomiej Zolnierkiewicz
-1 siblings, 0 replies; 77+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-07-24 8:28 UTC (permalink / raw)
To: Greg Kroah-Hartman, Daniel Vetter
Cc: Linux Fbdev development list, Tetsuo Handa, syzbot,
Linux Kernel Mailing List, dri-devel, George Kennedy,
Dmitry Vyukov, Jiri Slaby, Dan Carpenter
On 7/23/20 4:21 PM, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
>> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
>>>> On 2020/07/16 19:00, Daniel Vetter wrote:
>>>>> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>>>>>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>>>>>> I've complained about integer overflows in fbdev for a long time...
>>>>>>>
>>>>>>> What I'd like to see is something like the following maybe. I don't
>>>>>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>>>>>> for negative.
>>>>>>
>>>>>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>>>>>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>>>>>> we want basic checks. That's a task for fbdev people who should be familiar with
>>>>>> necessary constraints.
>>>>>
>>>>> I think the worldwide supply of people who understand fbdev and willing to
>>>>> work on it is roughly 0. So if someone wants to fix this mess properly
>>>>> (which likely means adding tons of over/underflow checks at entry points,
>>>>> since you're never going to catch the driver bugs, there's too many and
>>>>> not enough people who care) they need to fix this themselves.
>>>>
>>>> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
>>>> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
>>>> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
>>>> rows and cols < 32768 ?
>>>>
>>>>>
>>>>> Just to avoid confusion here.
>>>>>
>>>>>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>>>>>> of basic checks?
>>>>>
>>>>> Which two patches where?
>>>>
>>>> [PATCH v3] vt: Reject zero-sized screen buffer size.
>>>> from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> This is now in my tree.
>>>
>>>> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
>>>> from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> That should be taken by the fbdev maintainer, but I can take it too if
>>> people want.
>>
>> Just missed this weeks pull request train and feeling like not worth
>> making this an exception (it's been broken forever after all), so
>> maybe best if you just add this to vt.
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Also this avoids the impression I know what's going on in fbdev code,
>> maybe with sufficient abandon from my side someone will pop up who
>> cares an fixes the bazillion of syzkaller issues we seem to have
>> around console/vt and everything related.
>
> Great, will go queue it up now, thanks!
Fine with me, thanks!
PS I'll later queue the patch from George to drm-misc-next (after
reading both fbdev patches in detail it seems that both are needed).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-24 8:28 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 77+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-07-24 8:28 UTC (permalink / raw)
To: Greg Kroah-Hartman, Daniel Vetter
Cc: Tetsuo Handa, Dan Carpenter, Linux Fbdev development list, syzbot,
Linux Kernel Mailing List, dri-devel, George Kennedy, Jiri Slaby,
Dmitry Vyukov
On 7/23/20 4:21 PM, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
>> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
>>>> On 2020/07/16 19:00, Daniel Vetter wrote:
>>>>> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>>>>>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>>>>>> I've complained about integer overflows in fbdev for a long time...
>>>>>>>
>>>>>>> What I'd like to see is something like the following maybe. I don't
>>>>>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>>>>>> for negative.
>>>>>>
>>>>>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>>>>>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>>>>>> we want basic checks. That's a task for fbdev people who should be familiar with
>>>>>> necessary constraints.
>>>>>
>>>>> I think the worldwide supply of people who understand fbdev and willing to
>>>>> work on it is roughly 0. So if someone wants to fix this mess properly
>>>>> (which likely means adding tons of over/underflow checks at entry points,
>>>>> since you're never going to catch the driver bugs, there's too many and
>>>>> not enough people who care) they need to fix this themselves.
>>>>
>>>> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
>>>> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
>>>> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
>>>> rows and cols < 32768 ?
>>>>
>>>>>
>>>>> Just to avoid confusion here.
>>>>>
>>>>>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>>>>>> of basic checks?
>>>>>
>>>>> Which two patches where?
>>>>
>>>> [PATCH v3] vt: Reject zero-sized screen buffer size.
>>>> from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> This is now in my tree.
>>>
>>>> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
>>>> from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> That should be taken by the fbdev maintainer, but I can take it too if
>>> people want.
>>
>> Just missed this weeks pull request train and feeling like not worth
>> making this an exception (it's been broken forever after all), so
>> maybe best if you just add this to vt.
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Also this avoids the impression I know what's going on in fbdev code,
>> maybe with sufficient abandon from my side someone will pop up who
>> cares an fixes the bazillion of syzkaller issues we seem to have
>> around console/vt and everything related.
>
> Great, will go queue it up now, thanks!
Fine with me, thanks!
PS I'll later queue the patch from George to drm-misc-next (after
reading both fbdev patches in detail it seems that both are needed).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-24 8:28 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 77+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-07-24 8:28 UTC (permalink / raw)
To: Greg Kroah-Hartman, Daniel Vetter
Cc: Linux Fbdev development list, Tetsuo Handa, syzbot,
Linux Kernel Mailing List, dri-devel, George Kennedy,
Dmitry Vyukov, Jiri Slaby, Dan Carpenter
On 7/23/20 4:21 PM, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
>> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
>>>> On 2020/07/16 19:00, Daniel Vetter wrote:
>>>>> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>>>>>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>>>>>> I've complained about integer overflows in fbdev for a long time...
>>>>>>>
>>>>>>> What I'd like to see is something like the following maybe. I don't
>>>>>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>>>>>> for negative.
>>>>>>
>>>>>> Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at
>>>>>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329bce@i-love.sakura.ne.jp/ ,
>>>>>> we want basic checks. That's a task for fbdev people who should be familiar with
>>>>>> necessary constraints.
>>>>>
>>>>> I think the worldwide supply of people who understand fbdev and willing to
>>>>> work on it is roughly 0. So if someone wants to fix this mess properly
>>>>> (which likely means adding tons of over/underflow checks at entry points,
>>>>> since you're never going to catch the driver bugs, there's too many and
>>>>> not enough people who care) they need to fix this themselves.
>>>>
>>>> But I think we can enforce reasonable constraint which is much stricter than Dan's basic_checks()
>>>> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >= 1048576, for
>>>> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize() accepts only
>>>> rows and cols < 32768 ?
>>>>
>>>>>
>>>>> Just to avoid confusion here.
>>>>>
>>>>>> Anyway, my two patches are small and low cost; can we apply these patches regardless
>>>>>> of basic checks?
>>>>>
>>>>> Which two patches where?
>>>>
>>>> [PATCH v3] vt: Reject zero-sized screen buffer size.
>>>> from https://lkml.kernel.org/r/20200712111013.11881-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> This is now in my tree.
>>>
>>>> [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
>>>> from https://lkml.kernel.org/r/20200715015102.3814-1-penguin-kernel@I-love.SAKURA.ne.jp
>>>
>>> That should be taken by the fbdev maintainer, but I can take it too if
>>> people want.
>>
>> Just missed this weeks pull request train and feeling like not worth
>> making this an exception (it's been broken forever after all), so
>> maybe best if you just add this to vt.
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Also this avoids the impression I know what's going on in fbdev code,
>> maybe with sufficient abandon from my side someone will pop up who
>> cares an fixes the bazillion of syzkaller issues we seem to have
>> around console/vt and everything related.
>
> Great, will go queue it up now, thanks!
Fine with me, thanks!
PS I'll later queue the patch from George to drm-misc-next (after
reading both fbdev patches in detail it seems that both are needed).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-14 10:27 ` Tetsuo Handa
` (2 preceding siblings ...)
(?)
@ 2020-07-14 17:15 ` George Kennedy
2020-07-15 0:24 ` Tetsuo Handa
-1 siblings, 1 reply; 77+ messages in thread
From: George Kennedy @ 2020-07-14 17:15 UTC (permalink / raw)
To: Tetsuo Handa, Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
Dan Carpenter, Jiri Slaby, Dmitry Vyukov
[-- Attachment #1.1: Type: text/plain, Size: 9093 bytes --]
Hello Tetsuo,
Can you try the a.out built from the original Syzkaller modified repro C
program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct.
// https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
// autogenerated by syzkaller (https://github.com/google/syzkaller)
#define _GNU_SOURCE
#include <endian.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/fcntl.h>
#include <unistd.h>
#include <errno.h>
#include <linux/fb.h>
int verbose = 0;
void
dumpit(unsigned char *buf, int count, int addr)
{
int i, j;
char bp[256];
memset(bp, 0, 256);
for (i = j = 0; i < count; i++, j++) {
if (j == 16) {
j = 0;
printf("%s\n", bp);
memset(bp, 0, 256);
}
if (j == 0) {
sprintf(&bp[strlen(bp)], "%x: ", addr + i);
}
sprintf(&bp[strlen(bp)], "%02x ", buf[i]);
}
if (j != 0) {
printf("%s\n", bp);
}
}
uint64_t r[1] = {0xffffffffffffffff};
int main(int argc, char **argv)
{
syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 3ul, 0x32ul, -1, 0);
intptr_t res = 0;
uint32_t activate = FB_ACTIVATE_NOW;
struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x200001c0;
struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct fb_var_screeninfo *));
char *vp = (char *)varp;
int i, sum, rtn, c;
extern char *optarg;
int limit = 0, passes = 0;
unsigned int start_address = 0;
unsigned int pattern = 0;
int breakit = 1;
while ((c = getopt (argc, argv, "a:v")) != -1)
switch (c)
{
case 'a':
activate = strtol(optarg, 0, 0);
break;
case 'v':
verbose++;
break;
default:
fprintf(stderr, "\nusage: %s [-a <activate code>] [-v]\n\n", argv[0]);
return -1;
}
int fd = open("/dev/fb0", O_RDWR);
if (fd < 0) {
perror("open");
return 0;
}
printf("fd: %d\n", fd);
r[0] = fd;
rtn = syscall(__NR_ioctl, r[0], 0x4600ul, 0x200001c0ul);
if (rtn < 0) {
perror("ioctl");
fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno);
}
if (verbose) {
printf("FBIOGET_VSCREENINFO:\n");
dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo), 0x200001c0);
}
memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo));
fprintf(stderr, "activate = %d\n", activate);
varp->activate = activate;
if (verbose) {
printf("Pre FBIOPUT_VSCREENINFO:\n");
dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo), 0x200001c0);
sleep(2);
}
rtn = syscall(__NR_ioctl, r[0], 0x4601ul, 0x200001c0ul);
if (rtn < 0) {
perror("ioctl");
fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno);
}
limit = 2;
for (pattern = 0 ; pattern < 8 ; pattern++) {
unsigned long addr = 0x200001c0;
passes = 0;
printf("\nWalk START addr = 0x%x, Break pattern=%x\n", addr, pattern);
while (addr <= 0x2000025c) {
fprintf(stderr, "======================== %d: addr=%x ========================\n", passes, addr);
memcpy(varp, starting_varp, sizeof(struct fb_var_screeninfo));
*(uint32_t*)addr = pattern;
varp->activate = activate;
printf("Pre FBIOPUT_VSCREENINFO: pattern=%x\n", pattern);
dumpit((unsigned char *)vp, sizeof(struct fb_var_screeninfo), 0x200001c0);
sleep(3);
rtn = syscall(__NR_ioctl, r[0], 0x4601ul, 0x200001c0ul);
if (rtn < 0) {
perror("ioctl");
fprintf(stderr, "rtn=%d, errno=%d\n", rtn, errno);
}
addr += 4;
passes++;
if (passes == limit)
break;
}
}
close(fd);
return 0;
}
With my patch it gets output like the following:
[root@localhost ~]# ./fb_break
fd: 3
activate = 0
Walk START addr = 0x200001c0, Break pattern=0
======================== 0: addr=200001c0 ========================
Pre FBIOPUT_VSCREENINFO: pattern=0
200001c0: 00 00 00 00 00 03 00 00 00 04 00 00 00 03 00 00
200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00
200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00
200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00
20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00
20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ioctl: Invalid argument
rtn=-1, errno=22
======================== 1: addr=200001c4 ========================
Pre FBIOPUT_VSCREENINFO: pattern=0
200001c0: 00 04 00 00 00 00 00 00 00 04 00 00 00 03 00 00
200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00
200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00
200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00
20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00
20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ioctl: Invalid argument
rtn=-1, errno=22
Walk START addr = 0x200001c0, Break pattern=1
======================== 0: addr=200001c0 ========================
Pre FBIOPUT_VSCREENINFO: pattern=1
200001c0: 01 00 00 00 00 03 00 00 00 04 00 00 00 03 00 00
200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00
200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00
200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00
20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00
20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ioctl: Invalid argument
rtn=-1, errno=22
...
======================== 1: addr=200001c4 ========================
Pre FBIOPUT_VSCREENINFO: pattern=7
200001c0: 00 04 00 00 07 00 00 00 00 04 00 00 00 03 00 00
200001d0: 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00
200001e0: 10 00 00 00 08 00 00 00 00 00 00 00 08 00 00 00
200001f0: 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00
20000200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000210: 00 00 00 00 00 00 00 00 2c 01 00 00 90 01 00 00
20000220: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20000250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ioctl: Invalid argument
rtn=-1, errno=22
[root@localhost ~]#
Thank you,
George
On 7/14/2020 6:27 AM, Tetsuo Handa wrote:
> On 2020/07/14 16:22, Bartlomiej Zolnierkiewicz wrote:
>> How does this patch relate to:
>>
>> https://marc.info/?l=linux-fbdev&m=159415024816722&w=2
>>
>> ?
>>
>> It seems to address the same issue, I've added George and Dan to Cc:.
> George Kennedy's patch does not help for my case.
>
> You can try a.out built from
>
> ----------
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <linux/fb.h>
>
> int main(int argc, char *argv[])
> {
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, &var);
> var.xres = var.yres = 16;
> ioctl(fd, FBIOPUT_VSCREENINFO, &var);
> return 0;
> }
> ----------
>
> with a fault injection patch
>
> ----------
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1214,6 +1214,10 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
>
> if (new_screen_size > KMALLOC_MAX_SIZE)
> return -EINVAL;
> + if (!strcmp(current->comm, "a.out")) {
> + printk(KERN_INFO "Forcing memory allocation failure.\n");
> + return -ENOMEM;
> + }
> newscreen = kzalloc(new_screen_size, GFP_USER);
> if (!newscreen)
> return -ENOMEM;
> ----------
>
> . What my patch workarounds is cases when vc_do_resize() did not update vc->vc_{cols,rows} .
> Unless vc->vc_{cols,rows} are updated by vc_do_resize() in a way that avoids integer underflow at
>
> unsigned int rw = info->var.xres - (vc->vc_cols*cw);
> unsigned int bh = info->var.yres - (vc->vc_rows*ch);
>
> , this crash won't go away.
>
> [ 39.995757][ T2788] Forcing memory allocation failure.
> [ 39.996527][ T2788] BUG: unable to handle page fault for address: ffffa9d180d7b000
> [ 39.996529][ T2788] #PF: supervisor write access in kernel mode
> [ 39.996530][ T2788] #PF: error_code(0x0002) - not-present page
> [ 39.996531][ T2788] PGD 13a48c067 P4D 13a48c067 PUD 13a48d067 PMD 1324e4067 PTE 0
> [ 39.996547][ T2788] Oops: 0002 [#1] SMP
> [ 39.996550][ T2788] CPU: 2 PID: 2788 Comm: a.out Not tainted 5.8.0-rc5+ #757
> [ 39.996551][ T2788] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
> [ 39.996555][ T2788] RIP: 0010:bitfill_aligned+0x87/0x120 [cfbfillrect]
[-- Attachment #1.2: Type: text/html, Size: 10053 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
2020-07-14 17:15 ` [PATCH] " George Kennedy
2020-07-15 0:24 ` Tetsuo Handa
@ 2020-07-15 0:24 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 0:24 UTC (permalink / raw)
To: George Kennedy, Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
Dan Carpenter, Jiri Slaby, Dmitry Vyukov
On 2020/07/15 2:15, George Kennedy wrote:
> Can you try the a.out built from the original Syzkaller modified repro C program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct.
I'm not familiar with exploit code. What do you want to explain via this program?
> struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x200001c0;
> struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct fb_var_screeninfo *));
> memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo));
> memcpy(varp, starting_varp, sizeof(struct fb_var_screeninfo));
At least, I suspect there is a memory corruption bug in this program
because of malloc()ing only sizeof(struct fb_var_screeninfo *) bytes.
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 0:24 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 0:24 UTC (permalink / raw)
To: George Kennedy, Bartlomiej Zolnierkiewicz
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-fbdev, dri-devel,
Dmitry Vyukov, linux-kernel, Dan Carpenter
On 2020/07/15 2:15, George Kennedy wrote:
> Can you try the a.out built from the original Syzkaller modified repro C program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct.
I'm not familiar with exploit code. What do you want to explain via this program?
> struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x200001c0;
> struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct fb_var_screeninfo *));
> memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo));
> memcpy(varp, starting_varp, sizeof(struct fb_var_screeninfo));
At least, I suspect there is a memory corruption bug in this program
because of malloc()ing only sizeof(struct fb_var_screeninfo *) bytes.
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
@ 2020-07-15 0:24 ` Tetsuo Handa
0 siblings, 0 replies; 77+ messages in thread
From: Tetsuo Handa @ 2020-07-15 0:24 UTC (permalink / raw)
To: George Kennedy, Bartlomiej Zolnierkiewicz
Cc: linux-fbdev, Greg Kroah-Hartman, linux-kernel, dri-devel,
Dan Carpenter, Jiri Slaby, Dmitry Vyukov
On 2020/07/15 2:15, George Kennedy wrote:
> Can you try the a.out built from the original Syzkaller modified repro C program? It walks 0-7 through xres and yres of the fb_var_screeninfo struct.
I'm not familiar with exploit code. What do you want to explain via this program?
> struct fb_var_screeninfo *varp = (struct fb_var_screeninfo *)0x200001c0;
> struct fb_var_screeninfo *starting_varp = malloc(sizeof(struct fb_var_screeninfo *));
> memcpy(starting_varp, varp, sizeof(struct fb_var_screeninfo));
> memcpy(varp, starting_varp, sizeof(struct fb_var_screeninfo));
At least, I suspect there is a memory corruption bug in this program
because of malloc()ing only sizeof(struct fb_var_screeninfo *) bytes.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 77+ messages in thread