Re: [PATCH 1/1] tty: fix out-of-bounds access in tty_driver_lookup_tty()
From: Jiri Slaby
Date: Fri Dec 09 2022 - 02:17:58 EST
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@xxxxxxxxxx>
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@xxxxxxxxxxxxx>
---
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