* [PATCH 0/1] crash in tty layer when specifying invalid console=ttyX @ 2022-12-07 7:52 Sven Schnelle 2022-12-07 7:52 ` [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty() Sven Schnelle 0 siblings, 1 reply; 5+ messages in thread From: Sven Schnelle @ 2022-12-07 7:52 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby Cc: Christian Borntraeger, linux-kernel, linux-s390 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty() 2022-12-07 7:52 [PATCH 0/1] crash in tty layer when specifying invalid console=ttyX Sven Schnelle @ 2022-12-07 7:52 ` Sven Schnelle 2022-12-09 7:17 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: Sven Schnelle @ 2022-12-07 7:52 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby Cc: Christian Borntraeger, linux-kernel, linux-s390 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty() 2022-12-07 7:52 ` [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty() Sven Schnelle @ 2022-12-09 7:17 ` Jiri Slaby 2022-12-09 8:10 ` Sven Schnelle 0 siblings, 1 reply; 5+ messages in thread From: Jiri Slaby @ 2022-12-09 7:17 UTC (permalink / raw) To: Sven Schnelle, Greg Kroah-Hartman Cc: Christian Borntraeger, linux-kernel, linux-s390 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty() 2022-12-09 7:17 ` Jiri Slaby @ 2022-12-09 8:10 ` Sven Schnelle 2022-12-09 8:43 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: Sven Schnelle @ 2022-12-09 8:10 UTC (permalink / raw) To: Jiri Slaby Cc: Greg Kroah-Hartman, Christian Borntraeger, linux-kernel, linux-s390 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty() 2022-12-09 8:10 ` Sven Schnelle @ 2022-12-09 8:43 ` Jiri Slaby 0 siblings, 0 replies; 5+ messages in thread From: Jiri Slaby @ 2022-12-09 8:43 UTC (permalink / raw) To: Sven Schnelle Cc: Greg Kroah-Hartman, Christian Borntraeger, linux-kernel, linux-s390 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-12-09 8:43 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-07 7:52 [PATCH 0/1] crash in tty layer when specifying invalid console=ttyX Sven Schnelle 2022-12-07 7:52 ` [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty() Sven Schnelle 2022-12-09 7:17 ` Jiri Slaby 2022-12-09 8:10 ` Sven Schnelle 2022-12-09 8:43 ` Jiri Slaby
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox