This removes duplicated initialization of variables (after reordering
'c' initialization).

It will also allow for eliminating whole translation into a separate
function in the next patch.

Note that vc_state, vc_utf etc. are checked with every rescan now. But
they are immutable for non-control characters where rescan might be
only necessary.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-15-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

We continue cleaning up do_con_write. This (hopefully) makes the
inversion code obvious.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-14-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The code was too overcomplicated. Extract vc_sanitize_unicode to a
separate function and flatten the code. I believe the code is
straightforward now.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-13-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

do_con_write is complicated enough. Extract unicode handling to a
separate function. For do_con_write, 249 LOCs lowered to 183 lines.

Use diff -w -b to see the difference is neligible -- mostly whitespace
and use of 'return's instead of 'continue's.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-12-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Use bools for rescan and inverse. And true/false accordingly.

Use u8 for width instead of uint8_t.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-11-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

vc_translate is used only in vt.c, so move the definition from a header
there. Also, it used to be a macro, so be modern and make a static
inline from it. This makes the code actually readable.

And as a preparation for next patches, rename it to vc_translate_ascii.
vc_translate will be a wrapper for both unicode and this one.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-10-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

VT100ID is unused, but defined twice. Kill it.

VT102ID is used only in respond_ID. Define there a variable with proper
type and use that instead. Then drop both defines of VT102ID too.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-9-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Pass the length of a string to respond_string and use
tty_insert_flip_string instead of a loop with tty_insert_flip_char. This
simplifies the processing on the tty side.

The added strlens are optimized during constant folding and propagation
and the result are proper constants in assembly.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-8-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

vc_tab_stop is used as a bitmap, but defined as an unsigned int array.
Switch it to bitmap and convert all users to the bitmap interface.

Note the difference in behavior! We no longer mask the top 24 bits away
from x, hence we do not wrap tabs at 256th column. Instead, we silently
drop attempts to set a tab behind 256 columns. And we will also seek by
'\t' to the rightmost column, when behind that boundary. I do not think
the original behavior was desired and that someone relies on that. If
this turns out to be the case, we can change the added 'if's back to
masks here and there instead...

(Or we can increase the limit as fb consoles now have 240 chars here.
And they could have more with higher than my resolution, of course.)
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-6-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Declare Gx_charset[2] instead of G0_charset and G1_charset. It makes
the code simpler (without ternary operators).
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-5-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The code for setting G0 and G1 is duplicated -- for each of them. Move
the code to a separate function (vc_setGx) and distinguish the two cases
by a parameter.

Change if-else-if to switch which allows for slightly better
optimization (decision tree).
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-4-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The code currently uses bitfields to store true-false values. Switch all
of that to bools. Apart from the cleanup, it saves 20B of code as many
shifts, ANDs, and ORs became simple movzb's.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-3-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Introduce names (en enum) for 0, 1, and 2 constants. We now have
VCI_HALF_BRIGHT, VCI_NORMAL, and VCI_BOLD instead.

Apart from the cleanup,
1) the enum allows for better type checking, and
2) this saves some code. No more fiddling with bits is needed in
   assembly now. (OTOH, the structure is larger.)
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-2-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

There are two copies of some members of struct vc_data. This is because
we need to save them and restore later. Move these memebers to a
separate structure called vc_state. So now instead of members like:
  vc_x, vc_y and vc_saved_x, vc_saved_y
we have
  state and saved_state (of type: struct vc_state)
containing
  state.x, state.y and saved_state.x, saved_state.y

This change:
* makes clear what is saved & restored
* eases save & restore by using memcpy (see save_cur and restore_cur)

Finally, we document the newly added struct vc_state using kernel-doc.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200615074910.19267-1-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Convert comments that reference mmap_sem to reference mmap_lock instead.

[akpm@linux-foundation.org: fix up linux-next leftovers]
[akpm@linux-foundation.org: s/lockaphore/lock/, per Vlastimil]
[akpm@linux-foundation.org: more linux-next fixups, per Michel]
Signed-off-by: NMichel Lespinasse <walken@google.com>
Signed-off-by: NAndrew Morton <akpm@linux-foundation.org>
Reviewed-by: NVlastimil Babka <vbabka@suse.cz>
Reviewed-by: NDaniel Jordan <daniel.m.jordan@oracle.com>
Cc: Davidlohr Bueso <dbueso@suse.de>
Cc: David Rientjes <rientjes@google.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Jerome Glisse <jglisse@redhat.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Laurent Dufour <ldufour@linux.ibm.com>
Cc: Liam Howlett <Liam.Howlett@oracle.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ying Han <yinghan@google.com>
Link: http://lkml.kernel.org/r/20200520052908.204642-13-walken@google.comSigned-off-by: NLinus Torvalds <torvalds@linux-foundation.org>

When k_ascii is invoked several times in a row there is a potential for
signed integer overflow:

UBSAN: Undefined behaviour in drivers/tty/vt/keyboard.c:888:19 signed integer overflow:
10 * 1111111111 cannot be represented in type 'int'
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.11 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
 <IRQ>
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0xce/0x128 lib/dump_stack.c:118
 ubsan_epilogue+0xe/0x30 lib/ubsan.c:154
 handle_overflow+0xdc/0xf0 lib/ubsan.c:184
 __ubsan_handle_mul_overflow+0x2a/0x40 lib/ubsan.c:205
 k_ascii+0xbf/0xd0 drivers/tty/vt/keyboard.c:888
 kbd_keycode drivers/tty/vt/keyboard.c:1477 [inline]
 kbd_event+0x888/0x3be0 drivers/tty/vt/keyboard.c:1495

While it can be worked around by using check_mul_overflow()/
check_add_overflow(), it is better to introduce a separate flag to
signal that number pad is being used to compose a symbol, and
change type of the accumulator from signed to unsigned, thus
avoiding undefined behavior when it overflows.
Reported-by: NKyungtae Kim <kt0755@gmail.com>
Signed-off-by: NDmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: stable <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200525232740.GA262061@dtor-wsSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

By directly using kfree() in different places we risk missing one if
it is switched to using vfree(), especially if the corresponding
vmalloc() is hidden away within a common abstraction.

Oh wait, that's exactly what happened here.

So let's fix this by creating a common abstraction for the free case
as well.
Signed-off-by: NNicolas Pitre <nico@fluxnic.net>
Reported-by: syzbot+0bfda3ade1ee9288a1be@syzkaller.appspotmail.com
Fixes: 9a98e7a8 ("vt: don't use kmalloc() for the unicode screen buffer")
Cc: <stable@vger.kernel.org>
Reviewed-by: NSam Ravnborg <sam@ravnborg.org>
Link: https://lore.kernel.org/r/nycvar.YSQ.7.76.2005021043110.2671@knanqh.ubzrSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The code in vc_do_resize() bounds the memory allocation size to avoid
exceeding MAX_ORDER down the kzalloc() call chain and generating a
runtime warning triggerable from user space. However, not only is it
unwise to use a literal value here, but MAX_ORDER may also be
configurable based on CONFIG_FORCE_MAX_ZONEORDER.
Let's use KMALLOC_MAX_SIZE instead.

Note that prior commit bb1107f7 ("mm, slab: make sure that
KMALLOC_MAX_SIZE will fit into MAX_ORDER") the KMALLOC_MAX_SIZE value
could not be relied upon.
Signed-off-by: NNicolas Pitre <nico@fluxnic.net>
Cc: <stable@vger.kernel.org> # v4.10+

Link: https://lore.kernel.org/r/nycvar.YSQ.7.76.2003281702410.2671@knanqh.ubzrSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Even if the actual screen size is bounded in vc_do_resize(), the unicode
buffer is still a little more than twice the size of the glyph buffer
and may exceed MAX_ORDER down the kmalloc() path. This can be triggered
from user space.

Since there is no point having a physically contiguous buffer here,
let's avoid the above issue as well as reducing pressure on high order
allocations by using vmalloc() instead.
Signed-off-by: NNicolas Pitre <nico@fluxnic.net>
Cc: <stable@vger.kernel.org>
Acked-by: NSam Ravnborg <sam@ravnborg.org>
Link: https://lore.kernel.org/r/nycvar.YSQ.7.76.2003282214210.2671@knanqh.ubzrSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Let's put it to a separate function, named vc_selection_store_chars.
Again, this makes vc_do_selection a bit shorter and more readable.
Having 4 local variables instead of 12 (5.6-rc1) looks much better now.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200415093608.10348-2-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Handle these actions:
* poking console
* TIOCL_SELCLEAR
* TIOCL_SELMOUSEREPORT
* start/end precomputation
* clear_selection if the console changed
in a separate function, thus making __set_selection_kernel way shorter
and more readable. The function still needs dissection, but we are
approaching.

This includes introduction of vc_selection and renaming
__set_selection_kernel to vc_do_selection.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200415093608.10348-1-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

vt_in_use() dereferences console_driver->ttys[i] without proper locking.
This is broken because the tty can be closed and freed concurrently.

We could fix this by using 'READ_ONCE(console_driver->ttys[i]) != NULL'
and skipping the check of tty_struct::count.  But, looking at
console_driver->ttys[i] isn't really appropriate anyway because even if
it is NULL the tty can still be in the process of being closed.

Instead, fix it by making vt_in_use() require console_lock() and check
whether the vt is allocated and has port refcount > 1.  This works since
following the patch "vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use
virtual console" the port refcount is incremented while the vt is open.

Reproducer (very unreliable, but it worked for me after a few minutes):

	#include <fcntl.h>
	#include <linux/vt.h>

	int main()
	{
		int fd, nproc;
		struct vt_stat state;
		char ttyname[16];

		fd = open("/dev/tty10", O_RDONLY);
		for (nproc = 1; nproc < 8; nproc *= 2)
			fork();
		for (;;) {
			sprintf(ttyname, "/dev/tty%d", rand() % 8);
			close(open(ttyname, O_RDONLY));
			ioctl(fd, VT_GETSTATE, &state);
		}
	}

KASAN report:

	BUG: KASAN: use-after-free in vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
	BUG: KASAN: use-after-free in vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
	Read of size 4 at addr ffff888065722468 by task syz-vt2/132

	CPU: 0 PID: 132 Comm: syz-vt2 Not tainted 5.6.0-rc5-00130-g089b6d36 #13
	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
	Call Trace:
	 [...]
	 vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
	 vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
	 tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
	 [...]

	Allocated by task 136:
	 [...]
	 kzalloc include/linux/slab.h:669 [inline]
	 alloc_tty_struct+0x96/0x8a0 drivers/tty/tty_io.c:2982
	 tty_init_dev+0x23/0x350 drivers/tty/tty_io.c:1334
	 tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
	 tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
	 [...]

	Freed by task 41:
	 [...]
	 kfree+0xbf/0x200 mm/slab.c:3757
	 free_tty_struct+0x8d/0xb0 drivers/tty/tty_io.c:177
	 release_one_tty+0x22d/0x2f0 drivers/tty/tty_io.c:1468
	 process_one_work+0x7f1/0x14b0 kernel/workqueue.c:2264
	 worker_thread+0x8b/0xc80 kernel/workqueue.c:2410
	 [...]

Fixes: 4001d7b7 ("vt: push down the tty lock so we can see what is left to tackle")
Cc: <stable@vger.kernel.org> # v3.4+
Acked-by: NJiri Slaby <jslaby@suse.cz>
Signed-off-by: NEric Biggers <ebiggers@google.com>
Link: https://lore.kernel.org/r/20200322034305.210082-3-ebiggers@kernel.orgSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The VT_DISALLOCATE ioctl can free a virtual console while tty_release()
is still running, causing a use-after-free in con_shutdown().  This
occurs because VT_DISALLOCATE considers a virtual console's
'struct vc_data' to be unused as soon as the corresponding tty's
refcount hits 0.  But actually it may be still being closed.

Fix this by making vc_data be reference-counted via the embedded
'struct tty_port'.  A newly allocated virtual console has refcount 1.
Opening it for the first time increments the refcount to 2.  Closing it
for the last time decrements the refcount (in tty_operations::cleanup()
so that it happens late enough), as does VT_DISALLOCATE.

Reproducer:
	#include <fcntl.h>
	#include <linux/vt.h>
	#include <sys/ioctl.h>
	#include <unistd.h>

	int main()
	{
		if (fork()) {
			for (;;)
				close(open("/dev/tty5", O_RDWR));
		} else {
			int fd = open("/dev/tty10", O_RDWR);

			for (;;)
				ioctl(fd, VT_DISALLOCATE, 5);
		}
	}

KASAN report:
	BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
	Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129

	CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
	Call Trace:
	 [...]
	 con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
	 release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
	 tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
	 tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
	 [...]

	Allocated by task 129:
	 [...]
	 kzalloc include/linux/slab.h:669 [inline]
	 vc_allocate drivers/tty/vt/vt.c:1085 [inline]
	 vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
	 con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
	 tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
	 tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
	 tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
	 tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
	 [...]

	Freed by task 130:
	 [...]
	 kfree+0xbf/0x1e0 mm/slab.c:3757
	 vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
	 vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
	 tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
	 [...]

Fixes: 4001d7b7 ("vt: push down the tty lock so we can see what is left to tackle")
Cc: <stable@vger.kernel.org> # v3.4+
Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
Acked-by: NJiri Slaby <jslaby@suse.cz>
Signed-off-by: NEric Biggers <ebiggers@google.com>
Link: https://lore.kernel.org/r/20200322034305.210082-2-ebiggers@kernel.orgSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Shift cases one level left. This makes the code more readable and some
lines need not wrap anymore.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200316065911.11024-3-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

We have a helper called rounddown for these modulo computations. So use
it.

No functional change intended and "objdump -d" proves that.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200316065911.11024-2-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Instead of awkward ternary operator with comparison, use simple min()
for blankinterval and vesa_off_interval.

No functional change intended and "objdump -d" proves that.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200316065911.11024-1-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The merge commit cb05c6c8 (Merge 5.6-rc5 into tty-next) introduced a
double lock to set_selection_kernel. vc_sel.lock is locked both in
set_selection_kernel and its callee __set_selection_kernel now.

Remove the latter.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Link: https://lore.kernel.org/r/20200316064544.4799-1-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The vc_cons_allocated() checks in vt_ioctl() and vt_compat_ioctl() are
unnecessary because they can only be reached by calling ioctl() on an
open tty, which implies the corresponding virtual console is allocated.

And even if the virtual console *could* be freed concurrently, then
these checks would be broken since they aren't done under console_lock,
and the vc_data is dereferenced before them anyway.

So, remove these unneeded checks to avoid confusion.
Signed-off-by: NEric Biggers <ebiggers@google.com>
Link: https://lore.kernel.org/r/20200224080326.295046-1-ebiggers@kernel.orgSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

The might_sleep() in do_con_write() is redundant because console_lock()
already contains might_sleep().  Remove it.
Signed-off-by: NEric Biggers <ebiggers@google.com>
Link: https://lore.kernel.org/r/20200224073450.292892-1-ebiggers@kernel.orgSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Shift the cases one level left as this is how we are supposed to write
the switch-case code according to the CodingStyle.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-9-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

It is declared in vt_kern.h, so no need to declare it in selection.c
which includes the header.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-8-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

vt_dont_switch is pure boolean, no need for whole char.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-6-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Move all the selection global variables to a structure vc_selection,
instantiated as vc_sel. This helps to group all the variables together
and see what should be protected by the embedded lock too.

It might be used later also for per-console selection support.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-5-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

use_unicode needs not be global. It is used only in set_selection_kernel
and sel_pos (a callee). It is also always set there prior calling
sel_pos. So make use_unicode local and rename it to plain shorter
"unicode". Finally, propagate it to sel_pos via parameter.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-4-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

multiplier and mode are not actually needed:
* multiplier is used only in kmalloc_array, so use "use_unicode ? 4 : 1"
  directly
* mode is used only to assign a bool in this manner:
  if (cond)
    x = true;
  else
    x = false;
  So do "x = cond" directly.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-3-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

sel_lock cannot nest in the console lock. Thanks to syzkaller, the
kernel states firmly:

> WARNING: possible circular locking dependency detected
> 5.6.0-rc3-syzkaller #0 Not tainted
> ------------------------------------------------------
> syz-executor.4/20336 is trying to acquire lock:
> ffff8880a2e952a0 (&tty->termios_rwsem){++++}, at: tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
>
> but task is already holding lock:
> ffffffff89462e70 (sel_lock){+.+.}, at: paste_selection+0x118/0x470 drivers/tty/vt/selection.c:374
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (sel_lock){+.+.}:
>        mutex_lock_nested+0x1b/0x30 kernel/locking/mutex.c:1118
>        set_selection_kernel+0x3b8/0x18a0 drivers/tty/vt/selection.c:217
>        set_selection_user+0x63/0x80 drivers/tty/vt/selection.c:181
>        tioclinux+0x103/0x530 drivers/tty/vt/vt.c:3050
>        vt_ioctl+0x3f1/0x3a30 drivers/tty/vt/vt_ioctl.c:364

This is ioctl(TIOCL_SETSEL).
Locks held on the path: console_lock -> sel_lock

> -> #1 (console_lock){+.+.}:
>        console_lock+0x46/0x70 kernel/printk/printk.c:2289
>        con_flush_chars+0x50/0x650 drivers/tty/vt/vt.c:3223
>        n_tty_write+0xeae/0x1200 drivers/tty/n_tty.c:2350
>        do_tty_write drivers/tty/tty_io.c:962 [inline]
>        tty_write+0x5a1/0x950 drivers/tty/tty_io.c:1046

This is write().
Locks held on the path: termios_rwsem -> console_lock

> -> #0 (&tty->termios_rwsem){++++}:
>        down_write+0x57/0x140 kernel/locking/rwsem.c:1534
>        tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
>        mkiss_receive_buf+0x12aa/0x1340 drivers/net/hamradio/mkiss.c:902
>        tty_ldisc_receive_buf+0x12f/0x170 drivers/tty/tty_buffer.c:465
>        paste_selection+0x346/0x470 drivers/tty/vt/selection.c:389
>        tioclinux+0x121/0x530 drivers/tty/vt/vt.c:3055
>        vt_ioctl+0x3f1/0x3a30 drivers/tty/vt/vt_ioctl.c:364

This is ioctl(TIOCL_PASTESEL).
Locks held on the path: sel_lock -> termios_rwsem

> other info that might help us debug this:
>
> Chain exists of:
>   &tty->termios_rwsem --> console_lock --> sel_lock

Clearly. From the above, we have:
 console_lock -> sel_lock
 sel_lock -> termios_rwsem
 termios_rwsem -> console_lock

Fix this by reversing the console_lock -> sel_lock dependency in
ioctl(TIOCL_SETSEL). First, lock sel_lock, then console_lock.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Reported-by: syzbot+26183d9746e62da329b8@syzkaller.appspotmail.com
Fixes: 07e6124a ("vt: selection, close sel_buffer race")
Cc: stable <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200228115406.5735-2-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

We need to nest the console lock in sel_lock, so we have to push it down
a bit. Fortunately, the callers of set_selection_* just lock the console
lock around the function call. So moving it down is easy.

In the next patch, we switch the order.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Fixes: 07e6124a ("vt: selection, close sel_buffer race")
Cc: stable <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200228115406.5735-1-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

These two were macros. Switch them to static inlines, so that it's more
understandable what they are doing.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-2-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

Avoid global variables (namely sel_cons) by introducing vc_is_sel. It
checks whether the parameter is the current selection console. This will
help putting sel_cons to a struct later.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Link: https://lore.kernel.org/r/20200219073951.16151-1-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>

syzkaller reported this UAF:
BUG: KASAN: use-after-free in n_tty_receive_buf_common+0x2481/0x2940 drivers/tty/n_tty.c:1741
Read of size 1 at addr ffff8880089e40e9 by task syz-executor.1/13184

CPU: 0 PID: 13184 Comm: syz-executor.1 Not tainted 5.4.7 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
Call Trace:
...
 kasan_report+0xe/0x20 mm/kasan/common.c:634
 n_tty_receive_buf_common+0x2481/0x2940 drivers/tty/n_tty.c:1741
 tty_ldisc_receive_buf+0xac/0x190 drivers/tty/tty_buffer.c:461
 paste_selection+0x297/0x400 drivers/tty/vt/selection.c:372
 tioclinux+0x20d/0x4e0 drivers/tty/vt/vt.c:3044
 vt_ioctl+0x1bcf/0x28d0 drivers/tty/vt/vt_ioctl.c:364
 tty_ioctl+0x525/0x15a0 drivers/tty/tty_io.c:2657
 vfs_ioctl fs/ioctl.c:47 [inline]

It is due to a race between parallel paste_selection (TIOCL_PASTESEL)
and set_selection_user (TIOCL_SETSEL) invocations. One uses sel_buffer,
while the other frees it and reallocates a new one for another
selection. Add a mutex to close this race.

The mutex takes care properly of sel_buffer and sel_buffer_lth only. The
other selection global variables (like sel_start, sel_end, and sel_cons)
are protected only in set_selection_user. The other functions need quite
some more work to close the races of the variables there. This is going
to happen later.

This likely fixes (I am unsure as there is no reproducer provided) bug
206361 too. It was marked as CVE-2020-8648.
Signed-off-by: NJiri Slaby <jslaby@suse.cz>
Reported-by: syzbot+59997e8d5cbdc486e6f6@syzkaller.appspotmail.com
References: https://bugzilla.kernel.org/show_bug.cgi?id=206361
Cc: stable <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200210081131.23572-2-jslaby@suse.czSigned-off-by: NGreg Kroah-Hartman <gregkh@linuxfoundation.org>