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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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
Jiri Slaby <[email protected]> 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 <[email protected]>
>
> 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
On 09. 12. 22, 9:10, Sven Schnelle wrote:
> Jiri Slaby <[email protected]> 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 <[email protected]>
>>
>> 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