vgacon: fix out of bounds write to the scrollback buffer

PATCH Jiri Slaby 5 participants 10 messages Jul 30, 2020 – Aug 4, 2020 2 messages not in the archive

Subsystems (2)

Framebuffer layer Console

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jiri Slaby

author

2020-07-30 06:46 UTC #2 lore raw

Hi, OTOH, you should have CCed all the (public) lists.

Quoted 4 lines · 张云海

Zhang Xiao points out that the check should use > instead of >=, otherwise the last line will be skip.
I agree with that, so I modify the patch.
Could you please verify that it is still correct and sufficient?

IMO, yes, correct -- I was thinking about this yesterday too. Just an
example: hypothetically, if we had:
size_row = 1
tail = 29
size = 30

data[29] would be the last accessible member. Writing to data + tail (as "29 + 1 > 30" doesn't hold, so the modified check would pass), i.e. data[29] is still OK. So yes, > is OK, >= would waste space and would be actually incorrect.

Quoted 10 lines

BTW, Zhang Xiao also points out that the check after the memcpy can be remove.
I also think that was right, but vgacon_scrollback_cur->tail may keep the value vgacon_scrollback_cur->size in some case. That is not a problem in vgacon_scrollback_update because of the check before the memcpy. However, that may break some other code which assumes that vgacon_scrollback_cur->tail won't be vgacon_scrollback_cur->size. I do not know if there are such code, and if it is the code actually should check it too. But I still not remove the check in the patch to make sure it won't breaks other code.

As I wrote about this yesterday:
=I am also not sure the test I was pointing out on the top of this message would be of any use after the change. But maybe leave the code rest in peace.
=
I would let it as is in this particular code. Especially because vgacon_scrolldelta takes ->tail into consideration and I was too lazy to study the code there. But if you are willing to study the code there and confirm the check is superfluous, feel free to remove it. Perhaps in a separate patch. I was actually testing with the check removed and didn't hit any issue (which means, in fact, exactly nothing).

Quoted 6 lines

From ad143ede24ff4e61292cc9c96000100aacd97259 Mon Sep 17 00:00:00 2001
From: Yunhai Zhang <[email protected]>
Date: Tue, 28 Jul 2020 09:58:03 +0800
Subject: [PATCH] Fix for missing check in vgacon scrollback handling
vgacon_scrollback_update() always left enbough room in the scrollback

"leaves enough"

Quoted 2 lines

buffer for the next call, but if the console size changed that room might not actually be enough, and so we need to re-check.

Also, could you add reasoning why you are adding the check to the loop and not outside (for instance, use your reasoning with numbers or CSI M as an example).

Could you add a sample output here, something like I had:
= This leads to random crashes or KASAN reports like:

    BUG: KASAN: slab-out-of-bounds in vgacon_scroll+0x57a/0x8ed

=
It's then easier to google for when this happens to someone who runs non-patched kernels.

Quoted 13 lines

This fixes CVE-2020-14331.
Reported-and-debugged-by 张云海 <[email protected]>
Reported-and-debugged-by Yang Yingliang <[email protected]>
Reported-by Kyungtae Kim <[email protected]>
Fixes 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)
Cc Linus Torvalds <[email protected]>
Cc Greg KH <[email protected]>
Cc Solar Designer <[email protected]>
Cc "Srivatsa S. Bhat" <[email protected]>
Cc Anthony Liguori <[email protected]>
Cc Yang Yingliang <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>

Oh, and we should:
Cc: [email protected]

Quoted 10 lines

Signed-off-by Yunhai Zhang <[email protected]>
---
 drivers/video/console/vgacon.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/video/console/vgacon.c +0 -0
index 998b0de1812f..37b5711cd958 100644

drivers/video/console/vgacon.c static void vgacon_scrollback_update(struct vc_data *c, int t, int count) · line 260

Quoted 10 lines

drivers/video/console/vgacon.c +4 -0

@@ -251,6 +251,10 @@ static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
 	p = (void *) (c->vc_origin + t * c->vc_size_row);
 
 	while (count--) {
+		if ((vgacon_scrollback_cur->tail + c->vc_size_row) > 
+		    vgacon_scrollback_cur->size)
+			vgacon_scrollback_cur->tail = 0;
+
 		scr_memcpyw(vgacon_scrollback_cur->data +
 			    vgacon_scrollback_cur->tail,
 			    p, c->vc_size_row);

thanks,

Signature · 2 lines

js
suse labs

Jiri Slaby

author

2020-07-30 07:38 UTC #3 lore raw

Quoted 94 lines · Jiri Slaby

Hi, OTOH, you should have CCed all the (public) lists.
Quoted 4 lines · 张云海
Zhang Xiao points out that the check should use > instead of >=, otherwise the last line will be skip.
I agree with that, so I modify the patch.
Could you please verify that it is still correct and sufficient?
IMO, yes, correct -- I was thinking about this yesterday too. Just an
example: hypothetically, if we had:
size_row = 1
tail = 29
size = 30
data[29] would be the last accessible member. Writing to data + tail (as "29 + 1 > 30" doesn't hold, so the modified check would pass), i.e. data[29] is still OK. So yes, > is OK, >= would waste space and would be actually incorrect.
Quoted 10 lines
BTW, Zhang Xiao also points out that the check after the memcpy can be remove.
I also think that was right, but vgacon_scrollback_cur->tail may keep the value vgacon_scrollback_cur->size in some case. That is not a problem in vgacon_scrollback_update because of the check before the memcpy. However, that may break some other code which assumes that vgacon_scrollback_cur->tail won't be vgacon_scrollback_cur->size. I do not know if there are such code, and if it is the code actually should check it too. But I still not remove the check in the patch to make sure it won't breaks other code.
As I wrote about this yesterday:
=> I am also not sure the test I was pointing out on the top of this message would be of any use after the change. But maybe leave the code rest in peace.
=>
I would let it as is in this particular code. Especially because vgacon_scrolldelta takes ->tail into consideration and I was too lazy to study the code there. But if you are willing to study the code there and confirm the check is superfluous, feel free to remove it. Perhaps in a separate patch. I was actually testing with the check removed and didn't hit any issue (which means, in fact, exactly nothing).
Quoted 6 lines
From ad143ede24ff4e61292cc9c96000100aacd97259 Mon Sep 17 00:00:00 2001
From: Yunhai Zhang <[email protected]>
Date: Tue, 28 Jul 2020 09:58:03 +0800
Subject: [PATCH] Fix for missing check in vgacon scrollback handling
vgacon_scrollback_update() always left enbough room in the scrollback
"leaves enough"
Quoted 2 lines
buffer for the next call, but if the console size changed that room might not actually be enough, and so we need to re-check.
Also, could you add reasoning why you are adding the check to the loop and not outside (for instance, use your reasoning with numbers or CSI M as an example).
Could you add a sample output here, something like I had:
=> This leads to random crashes or KASAN reports like:
    BUG: KASAN: slab-out-of-bounds in vgacon_scroll+0x57a/0x8ed
=>
It's then easier to google for when this happens to someone who runs non-patched kernels.
Quoted 13 lines
This fixes CVE-2020-14331.
Reported-and-debugged-by 张云海 <[email protected]>
Reported-and-debugged-by Yang Yingliang <[email protected]>
Reported-by Kyungtae Kim <[email protected]>
Fixes 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)
Cc Linus Torvalds <[email protected]>
Cc Greg KH <[email protected]>
Cc Solar Designer <[email protected]>
Cc "Srivatsa S. Bhat" <[email protected]>
Cc Anthony Liguori <[email protected]>
Cc Yang Yingliang <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Oh, and we should:
Cc: [email protected]
Quoted 12 lines
Signed-off-by Yunhai Zhang <[email protected]>
---
 drivers/video/console/vgacon.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/video/console/vgacon.c +0 -0
index 998b0de1812f..37b5711cd958 100644
@@ -251,6 +251,10 @@ static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
 	p = (void *) (c->vc_origin + t * c->vc_size_row);
 

Quoted 2 lines

drivers/video/console/vgacon.c static void vgacon_scrollback_update(struct vc_data *c, int t, int count) · line 254
Quoted 2 lines
drivers/video/console/vgacon.c +1 -0
 	while (count--) {
+		if ((vgacon_scrollback_cur->tail + c->vc_size_row) > 

And git complains here:
.git/rebase-apply/patch:13: trailing whitespace.

                if ((vgacon_scrollback_cur->tail + c->vc_size_row) >

warning: 1 line adds whitespace errors.

There is a space at the EOL.

Quoted 9 lines

drivers/video/console/vgacon.c static void vgacon_scrollback_update(struct vc_data *c, int t, int count) · line 260
Quoted 6 lines
drivers/video/console/vgacon.c +3 -0
+		    vgacon_scrollback_cur->size)
+			vgacon_scrollback_cur->tail = 0;
+
 		scr_memcpyw(vgacon_scrollback_cur->data +
 			    vgacon_scrollback_cur->tail,
 			    p, c->vc_size_row);
thanks,

Signature · 1 line

js

张云海

2020-07-30 10:39 UTC #4 lore raw

In reply to Jiri Slaby

Update the patch, add CC list, sample output, and Jiri's PoC.

Quoted 1 line · Jiri Slaby

Hi, OTOH, you should have CCed all the (public) lists.

From ad143ede24ff4e61292cc9c96000100aacd97259 Mon Sep 17 00:00:00 2001
From: Yunhai Zhang <[email protected]>
Date: Tue, 28 Jul 2020 09:58:03 +0800
Subject: [PATCH] Fix for missing check in vgacon scrollback handling

vgacon_scrollback_update() always leaves enbough room in the scrollback buffer for the next call, but if the console size changed that room might not actually be enough, and so we need to re-check.

The check should be in the loop since vgacon_scrollback_cur->tail is updated in the loop and count may be more than 1 when triggered by CSI M, as Jiri's PoC:
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <fcntl.h>

int main(int argc, char** argv)

{
        int fd = open("/dev/tty1", O_RDWR);
        unsigned short size[3] = {25, 200, 0};
        ioctl(fd, 0x5609, size); // VT_RESIZE

        write(fd, "\e[1;1H", 6);
        for (int i = 0; i < 30; i++)
                write(fd, "\e[10M", 5);
}

It leads to various crashes as vgacon_scrollback_update writes out of the buffer:

 BUG: unable to handle page fault for address: ffffc900001752a0
 #PF: supervisor write access in kernel mode
 #PF: error_code(0x0002) - not-present page
 RIP: 0010:mutex_unlock+0x13/0x30
...
 Call Trace:
  n_tty_write+0x1a0/0x4d0
  tty_write+0x1a0/0x2e0

Or to KASAN reports:

BUG: KASAN: slab-out-of-bounds in vgacon_scroll+0x57a/0x8ed

This fixes CVE-2020-14331.

Reported-and-debugged-by 张云海 <[email protected]>

Reported-and-debugged-by Yang Yingliang <[email protected]>

Reported-by Kyungtae Kim <[email protected]>

Fixes 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)

Cc [email protected]

Cc Linus Torvalds <[email protected]>

Cc Greg KH <[email protected]>

Cc Solar Designer <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Cc Anthony Liguori <[email protected]>

Cc Yang Yingliang <[email protected]>

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Yunhai Zhang <[email protected]>

---
 drivers/video/console/vgacon.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/video/console/vgacon.c +4 -0

index 998b0de1812f..37b5711cd958 100644
@@ -251,6 +251,10 @@ static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
 	p = (void *) (c->vc_origin + t * c->vc_size_row);
 
 	while (count--) {
+		if ((vgacon_scrollback_cur->tail + c->vc_size_row) > 
+		    vgacon_scrollback_cur->size)
+			vgacon_scrollback_cur->tail = 0;
+
 		scr_memcpyw(vgacon_scrollback_cur->data +
 			    vgacon_scrollback_cur->tail,
 			    p, c->vc_size_row);

Signature · 1 line

2.25.1

张云海

2020-07-31 05:22 UTC #5 lore raw

Remove whitespace at EOL

Quoted 5 lines · 张云海

Update the patch, add CC list, sample output, and Jiri's PoC.
Quoted 1 line · Jiri Slaby
Hi, OTOH, you should have CCed all the (public) lists.

vgacon_scrollback_update() always leaves enbough room in the scrollback buffer for the next call, but if the console size changed that room might not actually be enough, and so we need to re-check.

int main(int argc, char** argv)

{
        int fd = open("/dev/tty1", O_RDWR);
        unsigned short size[3] = {25, 200, 0};
        ioctl(fd, 0x5609, size); // VT_RESIZE

        write(fd, "\e[1;1H", 6);
        for (int i = 0; i < 30; i++)
                write(fd, "\e[10M", 5);
}

It leads to various crashes as vgacon_scrollback_update writes out of the buffer:

 BUG: unable to handle page fault for address: ffffc900001752a0
 #PF: supervisor write access in kernel mode
 #PF: error_code(0x0002) - not-present page
 RIP: 0010:mutex_unlock+0x13/0x30
...
 Call Trace:
  n_tty_write+0x1a0/0x4d0
  tty_write+0x1a0/0x2e0

Or to KASAN reports:

BUG: KASAN: slab-out-of-bounds in vgacon_scroll+0x57a/0x8ed

This fixes CVE-2020-14331.

Reported-and-debugged-by 张云海 <[email protected]>

Reported-and-debugged-by Yang Yingliang <[email protected]>

Reported-by Kyungtae Kim <[email protected]>

Fixes 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)

Cc [email protected]

Cc Linus Torvalds <[email protected]>

Cc Greg KH <[email protected]>

Cc Solar Designer <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Cc Anthony Liguori <[email protected]>

Cc Yang Yingliang <[email protected]>

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Yunhai Zhang <[email protected]>

---
 drivers/video/console/vgacon.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/video/console/vgacon.c +4 -0

index 998b0de1812f..37b5711cd958 100644
@@ -251,6 +251,10 @@ static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
 	p = (void *) (c->vc_origin + t * c->vc_size_row);
 
 	while (count--) {
+		if ((vgacon_scrollback_cur->tail + c->vc_size_row) >
+		    vgacon_scrollback_cur->size)
+			vgacon_scrollback_cur->tail = 0;
+
 		scr_memcpyw(vgacon_scrollback_cur->data +
 			    vgacon_scrollback_cur->tail,
 			    p, c->vc_size_row);

Signature · 1 line

2.25.1

Jiri Slaby

author

2020-08-03 08:08 UTC #6 lore raw

Hi,

Quoted 1 line · ��ƺ�

Remove whitespace at EOL

I am fine with the patch. However it should be sent properly (inline mail, having a PATCH subject etc. -- see Documentation/process/submitting-patches.rst). git send-email after git format-patch handles most of it.

There is also question who is willing to take it?

Bart? Greg? Should we route it via akpm, or will you Linus directly? (I can sign off and resend the patch which was attached to the mail I am replying to too, if need be.)

thanks,

Signature · 2 lines

js
suse labs

Greg KH

2020-08-03 08:18 UTC #7 lore raw

Quoted 10 lines · Jiri Slaby

Hi,
Quoted 1 line · 张云海
Remove whitespace at EOL
I am fine with the patch. However it should be sent properly (inline mail, having a PATCH subject etc. -- see Documentation/process/submitting-patches.rst). git send-email after git format-patch handles most of it.

Quoted 5 lines

There is also question who is willing to take it?
Bart? Greg? Should we route it via akpm, or will you Linus directly? (I can sign off and resend the patch which was attached to the mail I am replying to too, if need be.)

I can take it, if Bart can't, just let me know.

thanks,

greg k-h

Daniel Vetter

2020-08-03 08:45 UTC #8 lore raw

Quoted 12 lines · Greg KH

Quoted 10 lines · Jiri Slaby
Hi,
Quoted 1 line · 张云海
Remove whitespace at EOL
I am fine with the patch. However it should be sent properly (inline mail, having a PATCH subject etc. -- see Documentation/process/submitting-patches.rst). git send-email after git format-patch handles most of it.

Quoted 7 lines

Quoted 5 lines
There is also question who is willing to take it?
Bart? Greg? Should we route it via akpm, or will you Linus directly? (I can sign off and resend the patch which was attached to the mail I am replying to too, if need be.)
I can take it, if Bart can't, just let me know.

Yeah vt stuff and console drivers != fbcon go through Greg's tree past few years ...

Greg, should we maybe add a MAINTAINERS entry that matches on all things console? With tty/vt you definitely have the other side of that coin already :-)
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Greg KH

2020-08-03 09:47 UTC #9 lore raw

Quoted 19 lines · Daniel Vetter

Quoted 18 lines · Greg KH
Quoted 15 lines · Jiri Slaby
Hi,
Quoted 1 line · 张云海
Remove whitespace at EOL
I am fine with the patch. However it should be sent properly (inline mail, having a PATCH subject etc. -- see Documentation/process/submitting-patches.rst). git send-email after git format-patch handles most of it.
There is also question who is willing to take it?
Bart? Greg? Should we route it via akpm, or will you Linus directly? (I can sign off and resend the patch which was attached to the mail I am replying to too, if need be.)

Quoted 8 lines

Quoted 1 line
I can take it, if Bart can't, just let me know.
Yeah vt stuff and console drivers != fbcon go through Greg's tree past few years ...
Greg, should we maybe add a MAINTAINERS entry that matches on all things console? With tty/vt you definitely have the other side of that coin already :-)

Sure, that would be good as things do fall through the cracks at times.

If you write the patch, I'll merge it :)

greg k-h

Bartlomiej Zolnierkiewicz

2020-08-03 11:07 UTC #10 lore raw

Quoted 22 lines · Greg KH

Quoted 21 lines · Daniel Vetter
Quoted 19 lines · Greg KH
Quoted 10 lines · Jiri Slaby
Hi,
Quoted 1 line · 张云海
Remove whitespace at EOL
I am fine with the patch. However it should be sent properly (inline mail, having a PATCH subject etc. -- see Documentation/process/submitting-patches.rst). git send-email after git format-patch handles most of it.
Quoted 5 lines
There is also question who is willing to take it?
Bart? Greg? Should we route it via akpm, or will you Linus directly? (I can sign off and resend the patch which was attached to the mail I am replying to too, if need be.)
I can take it, if Bart can't, just let me know.

Quoted 8 lines

Quoted 6 lines
Yeah vt stuff and console drivers != fbcon go through Greg's tree past few years ...
Greg, should we maybe add a MAINTAINERS entry that matches on all things console? With tty/vt you definitely have the other side of that coin already :-)
Sure, that would be good as things do fall through the cracks at times.

Since taking over fbdev in 2017 I've tried to act as the last resort Maintainer for console stuff (AFAIK there are no "lost" patches) but it really deserves its own entry.

Also most console patches make it through you nowadays anyway:

$ git log --pretty=fuller --since 17 drivers/video/console/|grep "Commit\:"|sort|uniq -cd
      2 Commit:     Arnd Bergmann <[email protected]>
     11 Commit:     Bartlomiej Zolnierkiewicz <[email protected]>
      2 Commit:     Daniel Vetter <[email protected]>
      3 Commit:     Dave Airlie <[email protected]>
     12 Commit:     Greg Kroah-Hartman <[email protected]>
      7 Commit:     Linus Torvalds <[email protected]>
      2 Commit:     Martin Schwidefsky <[email protected]>

Quoted 1 line

If you write the patch, I'll merge it :)

ACK from me. :)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Greg KH

2020-08-04 07:37 UTC #11 lore raw

In reply to Jiri Slaby

Quoted 9 lines · Jiri Slaby

Hi,
Quoted 1 line · 张云海
Remove whitespace at EOL
I am fine with the patch. However it should be sent properly (inline mail, having a PATCH subject etc. -- see Documentation/process/submitting-patches.rst). git send-email after git format-patch handles most of it.

I'll go fix this all up now "by hand" :(