Commit 52c3c3a5 authored Aug 24, 2020 by Jiri Slaby Committed by Greg Kroah-Hartman Aug 24, 2020

Revert "vc_screen: extract vcs_read_buf_header"



This reverts commit b1c32fcf, because
Syzkaller reports a use-after-free, a write in vcs_read:

BUG: KASAN: use-after-free in vcs_read_buf drivers/tty/vt/vc_screen.c:357 [inline]
BUG: KASAN: use-after-free in vcs_read+0xaa7/0xb40 drivers/tty/vt/vc_screen.c:449
Write of size 2 at addr ffff8880a8014000 by task syz-executor.5/16936
CPU: 1 PID: 16936 Comm: syz-executor.5 Not tainted 5.9.0-rc1-next-20200820-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
...
 kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
 vcs_read_buf drivers/tty/vt/vc_screen.c:357 [inline]
 vcs_read+0xaa7/0xb40 drivers/tty/vt/vc_screen.c:449

There are two issues with the patch:
1) vcs_read rounds the 'count' *up* to an even number. So if we read odd
   bytes from the header (3 bytes in the reproducer), the second byte of
   a (2-byte/ushort) write to temporary con_buf won't fit. It is because
   with the patch applied, we only subtract the real number read (3 bytes)
   and not the whole header (4 bytes).

2) in this scenario, we perform unaligned accesses now: there are
   2-byte/ushort writes to odd addresses. Due to the same reason as
   above.

Revert this for now, re-think and retry later.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Reported-by:  <syzbot+ad1f53726c3bd11180cb@syzkaller.appspotmail.com>
Fixes: b1c32fcf ("vc_screen: extract vcs_read_buf_header")
Cc: akpm@linux-foundation.org
Cc: nico@fluxnic.net
Link: https://lore.kernel.org/r/20200824095425.4376-1-jslaby@suse.cz


Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

parent 377c0d7e

drivers/tty/vt/vc_screen.c

+13 −18

Original line number	Diff line number	Diff line
		@@ -293,22 +293,6 @@ static void vcs_read_buf_noattr(const struct vc_data vc, char con_buf,
		}
		}

		static unsigned int vcs_read_buf_header(const struct vc_data vc, char con_buf,
		unsigned int pos, unsigned int count)
		{
		count = min(HEADER_SIZE - pos, count);

		/* clamp header values if they don't fit */
		con_buf[0] = min(vc->vc_rows, 0xFFu);
		con_buf[1] = min(vc->vc_cols, 0xFFu);
		getconsxy(vc, con_buf + 2);

		if (pos)
		memmove(con_buf, con_buf + pos, count);

		return count;
		}

		static unsigned int vcs_read_buf(const struct vc_data vc, char con_buf,
		unsigned int pos, unsigned int count, bool viewed,
		unsigned int *skip)
		@@ -318,11 +302,22 @@ static unsigned int vcs_read_buf(const struct vc_data vc, char con_buf,
		unsigned int filled = count;

		if (pos < HEADER_SIZE) {
		count -= vcs_read_buf_header(vc, con_buf, pos, count);
		/* clamp header values if they don't fit */
		con_buf[0] = min(vc->vc_rows, 0xFFu);
		con_buf[1] = min(vc->vc_cols, 0xFFu);
		getconsxy(vc, con_buf + 2);

		*skip += pos;
		count += pos;
		if (count > CON_BUF_SIZE) {
		count = CON_BUF_SIZE;
		filled = count - pos;
		}

		/* Advance state pointers and move on. */
		count -= min(HEADER_SIZE, count);
		pos = HEADER_SIZE;
		con_buf += HEADER_SIZE;

		/* If count >= 0, then pos is even... */
		} else if (pos & 1) {
		/*

Admin message