diff mbox series

tty: vt: saturate scrollback_delta to avoid overflow

Message ID 20240506-b4-sio-scrollback-delta-v1-1-4164d162a2b8@google.com
State New
Headers show
Series tty: vt: saturate scrollback_delta to avoid overflow | expand

Commit Message

Justin Stitt May 6, 2024, 6:55 p.m. UTC 
Using the signed overflow sanitizer with syzkaller produces this UBSAN
report:

[   31.304043] ------------[ cut here ]------------
[   31.304048] UBSAN: signed-integer-overflow in ../drivers/tty/vt/vt.c:309:19
[   31.304055] -2147483648 + -1073741824 cannot be represented in type 'int'
[   31.304066] CPU: 1 PID: 3894 Comm: syz-executor Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1
[   31.304073] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   31.304077] Call Trace:
[   31.304080]  <TASK>
[   31.304083]  dump_stack_lvl+0x93/0xd0
[   31.304177]  handle_overflow+0x171/0x1b0
[   31.304186]  scrollfront+0xcb/0xd0
[   31.304196]  tioclinux+0x3cc/0x450
[   31.304205]  tty_ioctl+0x7fc/0xc00
[   31.304212]  ? __pfx_tty_ioctl+0x10/0x10
[   31.304219]  __se_sys_ioctl+0xe0/0x140
[   31.304228]  do_syscall_64+0xd7/0x1b0
[   31.304236]  ? arch_exit_to_user_mode_prepare+0x11/0x60
[   31.304244]  entry_SYSCALL_64_after_hwframe+0x6f/0x77
[   31.304254] RIP: 0033:0x7fc3902ae539
[   31.304263] Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 8
[   31.304282] RSP: 002b:00007ffc8a457998 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   31.304289] RAX: ffffffffffffffda RBX: 00007fc3903e2f80 RCX: 00007fc3902ae539
[   31.304293] RDX: 0000000020000040 RSI: 000000000000541c RDI: 0000000000000003
[   31.304297] RBP: 00007fc39030d496 R08: 0000000000000000 R09: 0000000000000000
[   31.304300] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[   31.304304] R13: 0000000000000800 R14: 00007fc3903e2f80 R15: 00007fc3903e2f80
[   31.304310]  </TASK>
[   31.304371] ---[ end trace ]---

This is caused by the scrollback_delta overflowing. Historically, the
signed integer overflow sanitizer did not work in the kernel due to its
interaction with `-fwrapv` but this has since been changed [1] in the
newest version of Clang; It being re-enabled in the kernel with Commit
557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow sanitizer").

Note that it would be difficult to reproduce this bug in a non-fuzzing
scenario as it requires inputting tons of scroll inputs via keyboard
before the scheduled console callback has had a chance to update.
Nonetheless, let's saturate scrollback_delta so it stays clamped to
integer bounds without wrapping around.

[1]: https://github.com/llvm/llvm-project/pull/82432

Closes: https://github.com/KSPP/linux/issues/351
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: I am using Kees' SIO tree as a base:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=dev/v6.8-rc2/signed-overflow-sanitizer
---
 drivers/tty/vt/vt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)


---
base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc
change-id: 20240506-b4-sio-scrollback-delta-dff9aabace26

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Jiri Slaby May 9, 2024, 6:31 a.m. UTC | #1 
On 06. 05. 24, 20:55, Justin Stitt wrote:
> Using the signed overflow sanitizer with syzkaller produces this UBSAN
> report:
> 
> [   31.304043] ------------[ cut here ]------------
> [   31.304048] UBSAN: signed-integer-overflow in ../drivers/tty/vt/vt.c:309:19
> [   31.304055] -2147483648 + -1073741824 cannot be represented in type 'int'
> [   31.304066] CPU: 1 PID: 3894 Comm: syz-executor Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1
> [   31.304073] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> [   31.304077] Call Trace:
> [   31.304080]  <TASK>
> [   31.304083]  dump_stack_lvl+0x93/0xd0
> [   31.304177]  handle_overflow+0x171/0x1b0
> [   31.304186]  scrollfront+0xcb/0xd0
> [   31.304196]  tioclinux+0x3cc/0x450
> [   31.304205]  tty_ioctl+0x7fc/0xc00

The rest of the stack trace can be trimmed:

> [   31.304212]  ? __pfx_tty_ioctl+0x10/0x10
> [   31.304219]  __se_sys_ioctl+0xe0/0x140
> [   31.304228]  do_syscall_64+0xd7/0x1b0
> [   31.304236]  ? arch_exit_to_user_mode_prepare+0x11/0x60
> [   31.304244]  entry_SYSCALL_64_after_hwframe+0x6f/0x77
> [   31.304254] RIP: 0033:0x7fc3902ae539
> [   31.304263] Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 8
> [   31.304282] RSP: 002b:00007ffc8a457998 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [   31.304289] RAX: ffffffffffffffda RBX: 00007fc3903e2f80 RCX: 00007fc3902ae539
> [   31.304293] RDX: 0000000020000040 RSI: 000000000000541c RDI: 0000000000000003
> [   31.304297] RBP: 00007fc39030d496 R08: 0000000000000000 R09: 0000000000000000
> [   31.304300] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> [   31.304304] R13: 0000000000000800 R14: 00007fc3903e2f80 R15: 00007fc3903e2f80
> [   31.304310]  </TASK>
> [   31.304371] ---[ end trace ]---
> 
> This is caused by the scrollback_delta overflowing. Historically, the
> signed integer overflow sanitizer did not work in the kernel due to its
> interaction with `-fwrapv` but this has since been changed [1] in the
> newest version of Clang; It being re-enabled in the kernel with Commit
> 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow sanitizer").
> 
> Note that it would be difficult to reproduce this bug in a non-fuzzing
> scenario as it requires inputting tons of scroll inputs via keyboard
> before the scheduled console callback has had a chance to update.
> Nonetheless, let's saturate scrollback_delta so it stays clamped to
> integer bounds without wrapping around.

And what is actually broken, given signed overflow is well defined under 
the -fwrapv wings?

> [1]: https://github.com/llvm/llvm-project/pull/82432
> 
> Closes: https://github.com/KSPP/linux/issues/351
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Note: I am using Kees' SIO tree as a base:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=dev/v6.8-rc2/signed-overflow-sanitizer
> ---
>   drivers/tty/vt/vt.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 9b5b98dfc8b4..b4768336868e 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -308,7 +308,14 @@ static inline void scrolldelta(int lines)
>   	/* FIXME */
>   	/* scrolldelta needs some kind of consistency lock, but the BKL was
>   	   and still is not protecting versus the scheduled back end */
> -	scrollback_delta += lines;
> +
> +	/* saturate scrollback_delta so that it never wraps around */
> +	if (lines > 0 && unlikely(INT_MAX - lines < scrollback_delta))
> +		scrollback_delta = INT_MAX;
> +	else if (lines < 0 && unlikely(INT_MIN - lines > scrollback_delta))
> +		scrollback_delta = INT_MIN;
> +	else
> +		scrollback_delta += lines;

NACK, this is horrid.

Introduce a helper for this in overflow.h if we have none yet.

thanks,
Justin Stitt May 9, 2024, 10:01 p.m. UTC | #2 
Hi,

On Wed, May 8, 2024 at 11:31 PM Jiri Slaby <jirislaby@kernel.org> wrote:
>
>
> And what is actually broken, given signed overflow is well defined under
> the -fwrapv wings?
>

well-defined code can still be broken.

We want to better safeguard against accidental overflow as this is the
root cause of many bugs/vulnerabilities. So, in this spirit, we
recently enhanced the signed-integer-overflow sanitizer and its
ability to function with -fwrapv enabled [1]. On the Kernel side of
things, Kees' tree [2] has done a lot of the groundwork to get this
sanitizer enabled and less noisy.

WIth all that being said, my solution to this problem in this
particular instance is not the most elegant, as you rightly pointed
out.


> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 9b5b98dfc8b4..b4768336868e 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -308,7 +308,14 @@ static inline void scrolldelta(int lines)
> >       /* FIXME */
> >       /* scrolldelta needs some kind of consistency lock, but the BKL was
> >          and still is not protecting versus the scheduled back end */
> > -     scrollback_delta += lines;
> > +
> > +     /* saturate scrollback_delta so that it never wraps around */
> > +     if (lines > 0 && unlikely(INT_MAX - lines < scrollback_delta))
> > +             scrollback_delta = INT_MAX;
> > +     else if (lines < 0 && unlikely(INT_MIN - lines > scrollback_delta))
> > +             scrollback_delta = INT_MIN;
> > +     else
> > +             scrollback_delta += lines;
>
> NACK, this is horrid.

Agreed.

Does an implementation like this look any better?

static inline void scrolldelta(int lines)
{
        ...
        /* saturate scrollback_delta so that it never wraps around */
        if (lines > 0)
                scrollback_delta = min(scrollback_delta, INT_MAX -
lines) + lines;
        else
                scrollback_delta = max(scrollback_delta, INT_MIN -
lines) + lines;
        schedule_console_callback();
}

Accounting for the possibility of both underflow and overflow is what
is making this code a bit bloated but if this new approach looks good
enough I can send a v2.

>
> Introduce a helper for this in overflow.h if we have none yet.

We have these but for unsigned (size_t) types. I'm open to adding
signed_saturating_add(addend1, addend2, ceiling, floor) or something
similar.

>
> thanks,
> --
> js
> suse labs
>

[1]: https://github.com/llvm/llvm-project/pull/82432
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=dev/v6.8-rc2/signed-overflow-sanitizer

Thanks
Justin
Justin Stitt May 9, 2024, 10:04 p.m. UTC | #3 
On Thu, May 9, 2024 at 3:01 PM Justin Stitt <justinstitt@google.com> wrote:
>
>
> Agreed.
>
> Does an implementation like this look any better?
>
> static inline void scrolldelta(int lines)
> {
>         ...
>         /* saturate scrollback_delta so that it never wraps around */
>         if (lines > 0)
>                 scrollback_delta = min(scrollback_delta, INT_MAX -
> lines) + lines;
>         else
>                 scrollback_delta = max(scrollback_delta, INT_MIN -
> lines) + lines;
>         schedule_console_callback();
> }

I apologize for this formatting, gmail ate my tabs.

Note to self, do NOT copy/paste from vim to gmail's web client.

> Thanks
> Justin
gregkh@linuxfoundation.org May 10, 2024, 10:40 a.m. UTC | #4 
On Mon, May 06, 2024 at 06:55:19PM +0000, Justin Stitt wrote:
> Using the signed overflow sanitizer with syzkaller produces this UBSAN
> report:

<snip>

I think you might want to hold off on these until the discussion on the
hardening list about overflows all settles down to a solid resolution.
Right now these all seem to be a mess and perhaps you will have a better
set of primitives to work with once that thread is finished.

thanks,

greg k-h
Justin Stitt May 10, 2024, 4:13 p.m. UTC | #5 
On Fri, May 10, 2024 at 3:40 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, May 06, 2024 at 06:55:19PM +0000, Justin Stitt wrote:
> > Using the signed overflow sanitizer with syzkaller produces this UBSAN
> > report:
>
> <snip>
>
> I think you might want to hold off on these until the discussion on the
> hardening list about overflows all settles down to a solid resolution.
> Right now these all seem to be a mess and perhaps you will have a better
> set of primitives to work with once that thread is finished.

Agreed.

I came to the same conclusion once I reached fs/*.

Let's see how folks want to go forward with handling wraparound :thumbs_up:

>
> thanks,
>
> greg k-h

Thanks
Justin
diff mbox series

Patch

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9b5b98dfc8b4..b4768336868e 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -308,7 +308,14 @@  static inline void scrolldelta(int lines)
 	/* FIXME */
 	/* scrolldelta needs some kind of consistency lock, but the BKL was
 	   and still is not protecting versus the scheduled back end */
-	scrollback_delta += lines;
+
+	/* saturate scrollback_delta so that it never wraps around */
+	if (lines > 0 && unlikely(INT_MAX - lines < scrollback_delta))
+		scrollback_delta = INT_MAX;
+	else if (lines < 0 && unlikely(INT_MIN - lines > scrollback_delta))
+		scrollback_delta = INT_MIN;
+	else
+		scrollback_delta += lines;
 	schedule_console_callback();
 }