[PATCH 0/1] crash in tty layer when specifying invalid console=ttyX

[PATCH 0/1] crash in tty layer when specifying invalid console=ttyX

[Sven Schnelle]

Hi,

we had a user specifying 'console=tty3270' assuming that this will use the
tty3270 driver from s390 as console device. However, it will try to open
tty number 3270 as tty which is not what the user expected. That alone
isn't really a problem, but the kernel crashes while dereferencing invalid
memory with this option.

I tested this with qemu on x86, and it crashes in the same way. I never
worked in the tty layer, but it looks to me like there's some out-of-bound
checking missing in tty_driver_lookup_tty(). If this fix is wrong or
there's a better place to do that, let me know.

Sven Schnelle (1):
  tty: fix out-of-bounds access in tty_driver_lookup_tty()

 drivers/tty/tty_io.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.34.1

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

[Sven Schnelle]

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 ]---

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;
-- 
2.34.1

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

[Jiri Slaby]

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

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

[Sven Schnelle]

Jiri Slaby <jirislaby@kernel.org> writes:

> 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.
>> [..]
>
> 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

Makes sense. Should i add that to my patch, add a second patch, or
will you submit that?

Thanks
Sven

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

[Jiri Slaby]

On 09. 12. 22, 9:10, Sven Schnelle wrote:
> Jiri Slaby <jirislaby@kernel.org> writes:
> 
>> 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.
>>> [..]
>>
>> 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
> 
> Makes sense. Should i add that to my patch, add a second patch, or
> will you submit that?

If you can create a second patch, it would be great. And if you redo 
this one, please trim the stack traces (you can likely even drop the 
first one).

thanks,
-- 
js
suse labs