diff mbox series

[v2] tty: vt: always invoke vc->vc_sw->con_resize callback

Message ID 97f1d292-c3a8-f4d6-0651-b4f5571ecb72@i-love.sakura.ne.jp (mailing list archive)
State Superseded, archived
Headers show
Series [v2] tty: vt: always invoke vc->vc_sw->con_resize callback | expand

Commit Message

Tetsuo Handa May 15, 2021, 7:43 a.m. UTC 
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(-)

Comments

Maciej W. Rozycki May 15, 2021, 4:21 p.m. UTC | #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 <macro@orcam.me.uk>

  Maciej
Maciej W. Rozycki May 15, 2021, 4:32 p.m. UTC | #2 
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: stable@vger.kernel.org # v2.6.12+

  Maciej
Linus Torvalds May 15, 2021, 4:41 p.m. UTC | #3 
On Sat, May 15, 2021 at 9:33 AM Maciej W. Rozycki <macro@orcam.me.uk> 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: stable@vger.kernel.org # 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
Daniel Vetter May 17, 2021, 1:13 p.m. UTC | #4 
On Sat, May 15, 2021 at 6:42 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, May 15, 2021 at 9:33 AM Maciej W. Rozycki <macro@orcam.me.uk> 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: stable@vger.kernel.org # 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
diff mbox series

Patch

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);