vgacon: fix out of bounds write to the scrollback buffer

Commit Message

Jiri Slaby July 29, 2020, 7:02 a.m. UTC

The current vgacon's scroll up implementation uses a circural buffer
in vgacon_scrollback_cur. It always advances tail to prepare it for the
next write and caps it to zero if the next ->vc_size_row bytes won't fit.

But when we change the VT size (e.g. by VT_RESIZE) in the meantime, the new
line might not fit to the end of the scrollback buffer in the next
attempt to scroll. This 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

So check whether the line fits in the buffer and wrap if needed. Do it
before the loop as console_sem is held and ->vc_size_row cannot change
during the execution of vgacon_scrollback_cur. If it does change, we
need to ensure it does not change elsewhere, not here.

Also, we do not split the write of a line into chunks as that would
break the consumers of the buffer. They expect ->cnt, ->tail and ->size
to be in harmony and advanced by ->vc_size_row.

I found few reports of this in the past, some with patches included,
some even 2 years old:
https://lore.kernel.org/lkml/CAEAjamsJnG-=TSOwgRbbb3B9Z-PA63oWmNPoKYWQ=Z=[e-mail]/
https://lore.kernel.org/lkml/[e-mail]/

This fixes CVE-2020-14331.

Big thanks to guys mentioned in the Reported-and-debugged-by lines below
who actually found the root cause.

Signed-off-by: Jiri Slaby <[e-mail]>
Reported-and-debugged-by: 张云海 <[e-mail]>
Reported-and-debugged-by: Yang Yingliang <[e-mail]>
Reported-by: Kyungtae Kim <[e-mail]>
Fixes: 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)
Cc: Linus Torvalds <[e-mail]>
Cc: Greg KH <[e-mail]>
Cc: Solar Designer <[e-mail]>
Cc: "Srivatsa S. Bhat" <[e-mail]>
Cc: Anthony Liguori <[e-mail]>
Cc: Security Officers <[e-mail]>
Cc: [e-mail]
Cc: Yang Yingliang <[e-mail]>
Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/video/console/vgacon.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

张云海 July 29, 2020, 7:53 a.m. UTC | #1

Hi All,

This patch dosen't fix the issue, the check should be in the loop.

The change of the VT sze is before vgacon_scrollback_update, not in the
meantime.

Let's consider the following situation:
	suppose:
		vgacon_scrollback_cur->size is 65440
		vgacon_scrollback_cur->tail is 64960
		c->vc_size_row is 160
		count is 5
	
	Reset c->vc_size_row to 200 by VT_RESIZE, then call
vgacon_scrollback_update.
	
	This will pass the check, since (vgacon_scrollback_cur->tail +
c->vc_size_row)
	is 65160 which is less then vgacon_scrollback_cur->size(65440).

	However, in the 3rd iteration of the loop, vgacon_scrollback_cur->tail
is update
	to 65360, the memcpy will overflow.

To avoid overflow, the check should be
           if ((vgacon_scrollback_cur->tail + c->vc_size_row * count) >=

However, this will break the circular of the buffer, since all 5 lines
will be copy at the beginning.

To avoid break circular, we have to detect if wrap occurs, use a loop to
copy lines before
wrap, reset vgacon_scrollback_cur->tail to 0, then use another loop to
copy lines after wrap.
Of course the 2 loop can be combine into 2 memcpy, that will be similar
to Linus's patch.

Thus, I think the check should be in the loop.
The 2 check in the loop seems to be redundancy, Zhang Xiao from
Windriver suggest that the check after the memcpy can be remove.
I think he was right, but not very sure.
Thus, I suggest we discuss that too.

Regards,
Yunhai Zhang / NSFOCUS Security Team


On 2020/7/29 15:02, Jiri Slaby wrote:
> The current vgacon's scroll up implementation uses a circural buffer
> in vgacon_scrollback_cur. It always advances tail to prepare it for the
> next write and caps it to zero if the next ->vc_size_row bytes won't fit.
> 
> But when we change the VT size (e.g. by VT_RESIZE) in the meantime, the new
> line might not fit to the end of the scrollback buffer in the next
> attempt to scroll. This 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
> 
> So check whether the line fits in the buffer and wrap if needed. Do it
> before the loop as console_sem is held and ->vc_size_row cannot change
> during the execution of vgacon_scrollback_cur. If it does change, we
> need to ensure it does not change elsewhere, not here.
> 
> Also, we do not split the write of a line into chunks as that would
> break the consumers of the buffer. They expect ->cnt, ->tail and ->size
> to be in harmony and advanced by ->vc_size_row.
> 
> I found few reports of this in the past, some with patches included,
> some even 2 years old:
> https://lore.kernel.org/lkml/CAEAjamsJnG-=TSOwgRbbb3B9Z-PA63oWmNPoKYWQ=Z=[e-mail]/
> https://lore.kernel.org/lkml/[e-mail]/
> 
> This fixes CVE-2020-14331.
> 
> Big thanks to guys mentioned in the Reported-and-debugged-by lines below
> who actually found the root cause.
> 
> Signed-off-by: Jiri Slaby <[e-mail]>
> Reported-and-debugged-by: 张云海 <[e-mail]>
> Reported-and-debugged-by: Yang Yingliang <[e-mail]>
> Reported-by: Kyungtae Kim <[e-mail]>
> Fixes: 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)
> Cc: Linus Torvalds <[e-mail]>
> Cc: Greg KH <[e-mail]>
> Cc: Solar Designer <[e-mail]>
> Cc: "Srivatsa S. Bhat" <[e-mail]>
> Cc: Anthony Liguori <[e-mail]>
> Cc: Security Officers <[e-mail]>
> Cc: [e-mail]
> Cc: Yang Yingliang <[e-mail]>
> Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/video/console/vgacon.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index f0f3d573f848..13194bb246f8 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -250,6 +250,11 @@ static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
>  
>  	p = (void *) (c->vc_origin + t * c->vc_size_row);
>  
> +	/* vc_size_row might have changed by VT_RESIZE in the meantime */
> +	if ((vgacon_scrollback_cur->tail + c->vc_size_row) >=
> +			vgacon_scrollback_cur->size)
> +		vgacon_scrollback_cur->tail = 0;
> +
>  	while (count--) {
>  		scr_memcpyw(vgacon_scrollback_cur->data +
>  			    vgacon_scrollback_cur->tail,
>

Jiri Slaby July 29, 2020, 8:11 a.m. UTC | #2

Hi,

On 29. 07. 20, 9:53, 张云海 wrote:
> This patch dosen't fix the issue, the check should be in the loop.
> 
> The change of the VT sze is before vgacon_scrollback_update, not in the
> meantime.
> 
> Let's consider the following situation:
> 	suppose:
> 		vgacon_scrollback_cur->size is 65440
> 		vgacon_scrollback_cur->tail is 64960
> 		c->vc_size_row is 160
> 		count is 5
> 	
> 	Reset c->vc_size_row to 200 by VT_RESIZE, then call
> vgacon_scrollback_update.
> 	
> 	This will pass the check, since (vgacon_scrollback_cur->tail +
> c->vc_size_row)
> 	is 65160 which is less then vgacon_scrollback_cur->size(65440).
> 
> 	However, in the 3rd iteration of the loop, vgacon_scrollback_cur->tail
> is update
> 	to 65360, the memcpy will overflow.

But the loop checks for the overflow:
  if (vgacon_scrollback_cur->tail >= vgacon_scrollback_cur->size)
        vgacon_scrollback_cur->tail = 0;

So the first 2 iterations would write to the end of the buffer and this
3rd one should have zeroed ->tail.

thanks,

张云海 July 29, 2020, 8:19 a.m. UTC | #3

On 2020/7/29 16:11, Jiri Slaby wrote:
> But the loop checks for the overflow:
>   if (vgacon_scrollback_cur->tail >= vgacon_scrollback_cur->size)
>         vgacon_scrollback_cur->tail = 0;
> 
> So the first 2 iterations would write to the end of the buffer and this
> 3rd one should have zeroed ->tail.

In the 2nd  iteration before the check:
vgacon_scrollback_cur->tail is 65360 which is still less then
vgacon_scrollback_cur->size(65440), so the ->tail won't be zeroed.

Then it gose to the 3rd  iteration, overflow occurs.

Regards,
Yunhai Zhang / NSFOCUS Security Team

Jiri Slaby July 29, 2020, 11:20 a.m. UTC | #4

On 29. 07. 20, 10:19, 张云海 wrote:
> On 2020/7/29 16:11, Jiri Slaby wrote:
>> But the loop checks for the overflow:
>>   if (vgacon_scrollback_cur->tail >= vgacon_scrollback_cur->size)
>>         vgacon_scrollback_cur->tail = 0;
>>
>> So the first 2 iterations would write to the end of the buffer and this
>> 3rd one should have zeroed ->tail.
> 
> In the 2nd  iteration before the check:
> vgacon_scrollback_cur->tail is 65360 which is still less then
> vgacon_scrollback_cur->size(65440), so the ->tail won't be zeroed.
> 
> Then it gose to the 3rd  iteration, overflow occurs.

Ahh, I see now! So it must be triggered by CSI M instead. It allows for
more than 1 in count. So this is PoC for this case:
#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 corrupts memory, so it crashes the kernel randomly. Even with my
before-loop patch.

So now: could you resend your patch with improved commit message, add
all those Ccs etc.? You can copy most of the Ccs from my patch verbatim.

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.

thanks,

Bartlomiej Zolnierkiewicz July 29, 2020, 1:37 p.m. UTC | #5

Hi Jiri,

On 7/29/20 9:02 AM, Jiri Slaby wrote:
> The current vgacon's scroll up implementation uses a circural buffer
> in vgacon_scrollback_cur. It always advances tail to prepare it for the
> next write and caps it to zero if the next ->vc_size_row bytes won't fit.
> 
> But when we change the VT size (e.g. by VT_RESIZE) in the meantime, the new
> line might not fit to the end of the scrollback buffer in the next
> attempt to scroll. This 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
> 
> So check whether the line fits in the buffer and wrap if needed. Do it
> before the loop as console_sem is held and ->vc_size_row cannot change
> during the execution of vgacon_scrollback_cur. If it does change, we
> need to ensure it does not change elsewhere, not here.
> 
> Also, we do not split the write of a line into chunks as that would
> break the consumers of the buffer. They expect ->cnt, ->tail and ->size
> to be in harmony and advanced by ->vc_size_row.
> 
> I found few reports of this in the past, some with patches included,
> some even 2 years old:
> https://lore.kernel.org/lkml/CAEAjamsJnG-=TSOwgRbbb3B9Z-PA63oWmNPoKYWQ=Z=[e-mail]/

Sorry but I don't work on fixing fbdev/console KASAN/syzbot/etc.
reports (-ENORESOURCES).  This has been made official in the past.

I'm happy to review/apply patches though.

> https://lore.kernel.org/lkml/[e-mail]/

This was the first time the patch for issue was submitted.

I tried to apply it to drm-misc but then I have noticed that
it has not been posted to linux-fbdev / dri-devel MLs (so it
was not possible to merge it using dim tool) and thus I've
requested the author to resend it:

https://lore.kernel.org/lkml/[e-mail]/

which he did:

https://lore.kernel.org/lkml/[e-mail]/

and the patch is currently under review period (to give people
chance to comment on it) and in my "to apply if no objections"
folder.

I see that your/Yunhai patch addresses the root source of
the issue so I'll be happy to apply/ACK it instead of Yang's
patch once the final version is posted.

Thank you for working on this.

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

> This fixes CVE-2020-14331.
> 
> Big thanks to guys mentioned in the Reported-and-debugged-by lines below
> who actually found the root cause.
> 
> Signed-off-by: Jiri Slaby <[e-mail]>
> Reported-and-debugged-by: 张云海 <[e-mail]>
> Reported-and-debugged-by: Yang Yingliang <[e-mail]>
> Reported-by: Kyungtae Kim <[e-mail]>
> Fixes: 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)
> Cc: Linus Torvalds <[e-mail]>
> Cc: Greg KH <[e-mail]>
> Cc: Solar Designer <[e-mail]>
> Cc: "Srivatsa S. Bhat" <[e-mail]>
> Cc: Anthony Liguori <[e-mail]>
> Cc: Security Officers <[e-mail]>
> Cc: [e-mail]
> Cc: Yang Yingliang <[e-mail]>
> Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/video/console/vgacon.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index f0f3d573f848..13194bb246f8 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -250,6 +250,11 @@ static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
>  
>  	p = (void *) (c->vc_origin + t * c->vc_size_row);
>  
> +	/* vc_size_row might have changed by VT_RESIZE in the meantime */
> +	if ((vgacon_scrollback_cur->tail + c->vc_size_row) >=
> +			vgacon_scrollback_cur->size)
> +		vgacon_scrollback_cur->tail = 0;
> +
>  	while (count--) {
>  		scr_memcpyw(vgacon_scrollback_cur->data +
>  			    vgacon_scrollback_cur->tail,
>

Patch

diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index f0f3d573f848..13194bb246f8 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -250,6 +250,11 @@  static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
 
 	p = (void *) (c->vc_origin + t * c->vc_size_row);
 
+	/* vc_size_row might have changed by VT_RESIZE in the meantime */
+	if ((vgacon_scrollback_cur->tail + c->vc_size_row) >=
+			vgacon_scrollback_cur->size)
+		vgacon_scrollback_cur->tail = 0;
+
 	while (count--) {
 		scr_memcpyw(vgacon_scrollback_cur->data +
 			    vgacon_scrollback_cur->tail,