Re: [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty()

[Jiri Slaby <jirislaby@kernel.org>]

On 07. 12. 22, 8:52, Sven Schnelle wrote:
> When specifying an invalid console= device like console=tty3270,
> tty_driver_lookup_tty() returns the tty struct without checking
> whether index is a valid number.
> 
> To reproduce:
> 
> qemu-system-x86_64 -enable-kvm -nographic -serial mon:stdio \
> -kernel ../linux-build-x86/arch/x86/boot/bzImage \
> -append "console=ttyS0 console=tty3270"
> 
> This crashes with:
> 
> [    0.748921]   No soundcards found.
> [    0.749293] platform regulatory.0: Direct firmware load for regulatory.db failed with error -2
> [    0.750125] cfg80211: failed to load regulatory.db
> [    0.750611] ------------[ cut here ]------------
> [    0.751100] refcount_t: saturated; leaking memory.
> [    0.751603] WARNING: CPU: 0 PID: 1 at lib/refcount.c:22 refcount_warn_saturate+0x51/0x110
> [    0.752438] Modules linked in:
> [    0.752772] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc7-00194-gc911e8eba40a-dirty #15
> [    0.753609] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-2.fc37 04/01/2014
> [..]
> [    0.763352] Call Trace:
> [    0.763648]  <TASK>
> [    0.763907]  tty_open+0x5be/0x6f0
> [    0.764304]  chrdev_open+0xbd/0x230
> [    0.764694]  ? cdev_device_add+0x80/0x80
> [    0.765217]  do_dentry_open+0x1e0/0x410
> [    0.765719]  path_openat+0xca9/0x1050
> [    0.766224]  do_filp_open+0xaa/0x150
> [    0.766694]  file_open_name+0x133/0x1b0
> [    0.767260]  filp_open+0x27/0x50
> [    0.767674]  console_on_rootfs+0x14/0x4d
> [    0.768189]  kernel_init_freeable+0x1e4/0x20d
> [    0.768726]  ? rest_init+0xc0/0xc0
> [    0.769108]  kernel_init+0x11/0x120
> [    0.769480]  ret_from_fork+0x22/0x30
> [    0.769863]  </TASK>
> [    0.770128] ---[ end trace 0000000000000000 ]---
> [    0.770599] BUG: kernel NULL pointer dereference, address: 00000000000000ef
> [    0.771265] #PF: supervisor read access in kernel mode
> [    0.771773] #PF: error_code(0x0000) - not-present page
> [    0.772311] PGD 0 P4D 0·
> [    0.772609] Oops: 0000 [#1] PREEMPT SMP PTI
> [    0.773066] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.1.0-rc7-00194-gc911e8eba40a-dirty #15
> [    0.774027] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-2.fc37 04/01/2014
> [    0.774878] RIP: 0010:tty_open+0x268/0x6f0
> [..]
> [    0.783386] Call Trace:
> [    0.783715]  <TASK>
> [    0.784013]  chrdev_open+0xbd/0x230
> [    0.784444]  ? cdev_device_add+0x80/0x80
> [    0.784920]  do_dentry_open+0x1e0/0x410
> [    0.785389]  path_openat+0xca9/0x1050
> [    0.785813]  do_filp_open+0xaa/0x150
> [    0.786240]  file_open_name+0x133/0x1b0
> [    0.786746]  filp_open+0x27/0x50
> [    0.787244]  console_on_rootfs+0x14/0x4d
> [    0.787800]  kernel_init_freeable+0x1e4/0x20d
> [    0.788383]  ? rest_init+0xc0/0xc0
> [    0.788881]  kernel_init+0x11/0x120
> [    0.789356]  ret_from_fork+0x22/0x30
> [    0.789842]  </TASK>
> [    0.790163] Modules linked in:
> [    0.790502] CR2: 00000000000000ef
> [    0.790861] ---[ end trace 0000000000000000 ]---
> [    0.791332] RIP: 0010:tty_open+0x268/0x6f0
> [..]
> [    0.799648] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
> [    0.800479] Kernel Offset: 0x10400000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
> [    0.801534] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

Yes, this makes sense as a sanity check for all drivers. But I would 
_also_ disallow registering such a console in vt:
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3075,6 +3075,11 @@ int vt_kmsg_redirect(int new)
   * The console must be locked when we get here.
   */

+static int vt_console_setup(struct console *co, char *options)
+{
+       return co->index >= MAX_NR_CONSOLES ? -EINVAL : 0;
+}
+
  static void vt_console_print(struct console *co, const char *b, 
unsigned count)
  {
         struct vc_data *vc = vc_cons[fg_console].d;
@@ -3158,6 +3163,7 @@ static struct tty_driver *vt_console_device(struct 
console *c, int *index)

  static struct console vt_console_driver = {
         .name           = "tty",
+       .setup          = vt_console_setup,
         .write          = vt_console_print,
         .device         = vt_console_device,
         .unblank        = unblank_screen,

That means dmesg would say:
   Console: colour dummy device 80x25
   printk: console [ttyS0] enabled

And not:
   Console: colour dummy device 80x25
   printk: console [tty3270] enabled
   printk: console [ttyS0] enabled

> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>   drivers/tty/tty_io.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index de06c3c2ff70..1ac6784ea1f9 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1224,14 +1224,16 @@ static struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver,
>   {
>   	struct tty_struct *tty;
>   
> -	if (driver->ops->lookup)
> +	if (driver->ops->lookup) {
>   		if (!file)
>   			tty = ERR_PTR(-EIO);
>   		else
>   			tty = driver->ops->lookup(driver, file, idx);
> -	else
> +	} else {
> +		if (idx >= driver->num)
> +			return ERR_PTR(-EINVAL);
>   		tty = driver->ttys[idx];
> -
> +	}
>   	if (!IS_ERR(tty))
>   		tty_kref_get(tty);
>   	return tty;

thanks,
-- 
js
suse labs