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 <[email protected]>
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 <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Tetsuo Handa <[email protected]>
Tested-by: syzbot <[email protected]>
---
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);
--
2.25.1
On Sat, 15 May 2021, Tetsuo Handa wrote:
> 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);
LGTM, although I'll yet try to verify it with hardware. But it'll have
to wait another week or so as I'm currently away from my lab and this
requires physical presence.
Reviewed-by: Maciej W. Rozycki <[email protected]>
Maciej
On Sat, 15 May 2021, Maciej W. Rozycki wrote:
> On Sat, 15 May 2021, Tetsuo Handa wrote:
>
> > 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);
>
> LGTM, although I'll yet try to verify it with hardware. But it'll have
> to wait another week or so as I'm currently away from my lab and this
> requires physical presence.
NB I suggest that you request your change to be backported, i.e. post v3
with:
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: [email protected] # v2.6.12+
Maciej
On Sat, May 15, 2021 at 9:33 AM Maciej W. Rozycki <[email protected]> wrote:
>
> NB I suggest that you request your change to be backported, i.e. post v3
> with:
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: [email protected] # v2.6.12+
I've applied it to my tree, but let's wait to see that it doesn't
cause any issues before notifying the stable people.
Linus
On Sat, May 15, 2021 at 6:42 PM Linus Torvalds
<[email protected]> wrote:
>
> On Sat, May 15, 2021 at 9:33 AM Maciej W. Rozycki <[email protected]> wrote:
> >
> > NB I suggest that you request your change to be backported, i.e. post v3
> > with:
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: [email protected] # v2.6.12+
>
> I've applied it to my tree, but let's wait to see that it doesn't
> cause any issues before notifying the stable people.
Ah I missed all the fun with the long w/e. fwiw I think this looks
very reasonable, see my other reply why I think this shouldn't cause
issues. Especially when fbcon_resize only touches hw when in KD_TEXT
mode.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch