On 2021/05/15 1:19, Tetsuo Handa wrote:
Even if it turns out to be safe to always call this callback, we will need to involve another callback via "struct fb_ops" for checking the upper limits from fbcon_resize(). As a result, we will need to modify
drivers/tty/vt/vt.c drivers/video/fbdev/core/fbcon.c drivers/video/fbdev/vga16fb.c include/linux/fb.h
files only for checking rows/columns values passed to ioctl(VT_RESIZE) request.
I was by error assuming that fbcon_resize() cannot reject bogus rows/columns and thus we need to add another callback via "struct fb_ops" for that purpose. But fbcon_resize() does reject bogus rows/columns; it was simply because resize_screen() did not call fbcon_resize() if vc->vc_mode == KD_GRAPHICS. Thus, removing vc->vc_mode check alone is sufficient.
On 2021/05/15 6:10, Linus Torvalds wrote:
So I think just removing the "vc->vc_mode != KD_GRAPHICS" test from resize_screen() might be the way to go. That way, the low-level data structures actually are in sync with the resize, and the "out of bounds" bug should never happen.
Would you mind testing that?
OK. Your suggested changes passed the test by me and by syzbot.
From e5e326c90c5b919c6aba30072d665a00b18715a5 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Date: Sat, 15 May 2021 03:00:37 +0000 Subject: [PATCH v2] tty: vt: always invoke vc->vc_sw->con_resize callback
syzbot is reporting OOB write at vga16fb_imageblit() [1], for resize_screen() from ioctl(VT_RESIZE) returns 0 without checking whether requested rows/columns fit the amount of memory reserved for the graphical screen if current mode is KD_GRAPHICS.
---------- #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> #include <sys/ioctl.h> #include <linux/kd.h> #include <linux/vt.h>
int main(int argc, char *argv[]) { const int fd = open("/dev/char/4:1", O_RDWR); struct vt_sizes vt = { 0x4100, 2 };
ioctl(fd, KDSETMODE, KD_GRAPHICS); ioctl(fd, VT_RESIZE, &vt); ioctl(fd, KDSETMODE, KD_TEXT); return 0; } ----------
Allow framebuffer drivers to return -EINVAL, by moving vc->vc_mode != KD_GRAPHICS check from resize_screen() to fbcon_resize().
[1] https://syzkaller.appspot.com/bug?extid=1f29e126cf461c4de3b3
Reported-by: syzbot syzbot+1f29e126cf461c4de3b3@syzkaller.appspotmail.com Suggested-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Tested-by: syzbot syzbot+1f29e126cf461c4de3b3@syzkaller.appspotmail.com --- drivers/tty/vt/vt.c | 2 +- drivers/video/fbdev/core/fbcon.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 01645e87b3d5..fa1548d4f94b 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -1171,7 +1171,7 @@ static inline int resize_screen(struct vc_data *vc, int width, int height, /* Resizes the resolution of the display adapater */ int err = 0;
- if (vc->vc_mode != KD_GRAPHICS && vc->vc_sw->con_resize) + if (vc->vc_sw->con_resize) err = vc->vc_sw->con_resize(vc, width, height, user);
return err; diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 3406067985b1..22bb3892f6bd 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -2019,7 +2019,7 @@ static int fbcon_resize(struct vc_data *vc, unsigned int width, return -EINVAL;
pr_debug("resize now %ix%i\n", var.xres, var.yres); - if (con_is_visible(vc)) { + if (con_is_visible(vc) && vc->vc_mode == KD_TEXT) { var.activate = FB_ACTIVATE_NOW | FB_ACTIVATE_FORCE; fb_set_var(info, &var);