diff mbox series

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

Message ID 20221207075236.23171-2-svens@linux.ibm.com
State New
Headers
Series crash in tty layer when specifying invalid console=ttyX |

Commit Message

Sven Schnelle Dec. 7, 2022, 7:52 a.m. UTC 
  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(-)

Comments

Jiri Slaby Dec. 9, 2022, 7:17 a.m. UTC | #1 
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,
Sven Schnelle Dec. 9, 2022, 8:10 a.m. UTC | #2 
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
Jiri Slaby Dec. 9, 2022, 8:43 a.m. UTC | #3 
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,
diff mbox series

Patch

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;