linux.git/drivers/tty/vt/vc_screen.c, branch v6.0

Revert "vc_screen: extract vcs_read_buf_header"

2020-08-24T17:51:56+00:00

This reverts commit b1c32fcfadf5593ab7a63261cc8a5747c36e627e, 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 
Reported-by: syzbot+ad1f53726c3bd11180cb@syzkaller.appspotmail.com
Fixes: b1c32fcfadf5 ("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

vc_screen: prune macros

2020-08-18T11:45:22+00:00

Do not undefine random words. I guess this was here as there were macros
with such generic names somewhere. I very doubt they still exist. So
drop these.

And remove a spare blank line.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-16-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vc_screen: extract vcs_read_buf_header

2020-08-18T11:45:22+00:00

The attribute header handling is terrible in vcs_read_buf. Separate it
to a new function and simply do memmove (of up to 4 bytes) to the start
of the con_buf -- if user seeked.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-15-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vc_screen: extract vcs_read_buf

2020-08-18T11:45:22+00:00

And finally, move the attributes buffer handling to a separate function.
Leaving vcs_read quite compact.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-14-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vc_screen: extract vcs_read_buf_noattr

2020-08-18T11:45:22+00:00

Now, move the code for no-attributes handling to a separate function.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-13-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vc_screen: extract vcs_read_buf_uni

2020-08-18T11:45:21+00:00

The same as making write more readable, extract unicode handling from
vcs_read. The other two cases (w/ and w/o attributes) will follow.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-12-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vs_screen: kill tmp_count from vcs_read

2020-08-18T11:45:21+00:00

Both tmp_count computations and the single use can be eliminated using
min(). Do so.

Side note: we need HEADER_SIZE to be unsigned for min() not to complain.
Fix that too as all its other uses do not mind.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-11-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vc_screen: sanitize types in vcs_read

2020-08-18T11:45:21+00:00

* pos is derived from the passed ppos, so make it long enough, i.e.
  loff_t
* attr and uni_mode are booleans, so...
* size is limited by vcs_size() which returns an int
* read, p, orig_count and this_round are always ">= 0" and "< size",
  so uint is enough
* row, col, and max_col are derived from vc->vc_cols (uint) and p, so
  make them uint too
* tmp_count is derived from this_round, so make it an uint too.
* use u16 * for org (instead of unsigned short *). No need to initialize
  org too.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-10-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vc_screen: eliminate ifdefs from vcs_write_buf

2020-08-18T11:45:21+00:00

Introduce a new inline function called vc_compile_le16 and do the shifts
and ORs there. Depending on LE x BE.

I tried cpu_to_le16, but it ends up with worse assembly on BE for
whatever reason -- the compiler seems to be unable to optimize the swap.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-9-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman

vc_screen: extract vcs_write_buf

2020-08-18T11:45:21+00:00

This is the counterpart of the previous patch: here, we extract buffer
writing with attributes from vcs_write.

Now, there is no need for org to be initialized to NULL. The org0
check before update_region() confuses compilers, so check org instead.
It provides the same semantics. And it also eliminates the need for
initialization of org0.

We switch the branches of the attr 'if' too, as the inversion brings only
confusion now.

Signed-off-by: Jiri Slaby 
Link: https://lore.kernel.org/r/20200818085706.12163-8-jslaby@suse.cz
Signed-off-by: Greg Kroah-Hartman