From: Eric Biggers <[email protected]>
vt_in_use() dereferences console_driver->ttys[i] without proper locking.
This is broken because the tty can be closed and freed concurrently.
We could fix this by using 'READ_ONCE(console_driver->ttys[i]) != NULL'
and skipping the check of tty_struct::count. But, looking at
console_driver->ttys[i] isn't really appropriate anyway because even if
it is NULL the tty can still be in the process of being closed.
Instead, fix it by making vt_in_use() require console_lock() and check
whether the vt is allocated and has port refcount > 1. This works since
following the patch "vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use
virtual console" the port refcount is incremented while the vt is open.
Reproducer (very unreliable, but it worked for me after a few minutes):
#include <fcntl.h>
#include <linux/vt.h>
int main()
{
int fd, nproc;
struct vt_stat state;
char ttyname[16];
fd = open("/dev/tty10", O_RDONLY);
for (nproc = 1; nproc < 8; nproc *= 2)
fork();
for (;;) {
sprintf(ttyname, "/dev/tty%d", rand() % 8);
close(open(ttyname, O_RDONLY));
ioctl(fd, VT_GETSTATE, &state);
}
}
KASAN report:
BUG: KASAN: use-after-free in vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
BUG: KASAN: use-after-free in vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
Read of size 4 at addr ffff888065722468 by task syz-vt2/132
CPU: 0 PID: 132 Comm: syz-vt2 Not tainted 5.6.0-rc5-00130-g089b6d3654916 #13
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
Call Trace:
[...]
vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
[...]
Allocated by task 136:
[...]
kzalloc include/linux/slab.h:669 [inline]
alloc_tty_struct+0x96/0x8a0 drivers/tty/tty_io.c:2982
tty_init_dev+0x23/0x350 drivers/tty/tty_io.c:1334
tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
[...]
Freed by task 41:
[...]
kfree+0xbf/0x200 mm/slab.c:3757
free_tty_struct+0x8d/0xb0 drivers/tty/tty_io.c:177
release_one_tty+0x22d/0x2f0 drivers/tty/tty_io.c:1468
process_one_work+0x7f1/0x14b0 kernel/workqueue.c:2264
worker_thread+0x8b/0xc80 kernel/workqueue.c:2410
[...]
Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
Cc: <[email protected]> # v3.4+
Signed-off-by: Eric Biggers <[email protected]>
---
drivers/tty/vt/vt_ioctl.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index f62f498f63c05..ca0e57b1c0b43 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -43,9 +43,11 @@ bool vt_dont_switch;
static inline bool vt_in_use(unsigned int i)
{
- extern struct tty_driver *console_driver;
+ const struct vc_data *vc = vc_cons[i].d;
- return console_driver->ttys[i] && console_driver->ttys[i]->count;
+ WARN_CONSOLE_UNLOCKED();
+
+ return vc && kref_read(&vc->port.kref) > 1;
}
static inline bool vt_busy(int i)
@@ -643,15 +645,16 @@ int vt_ioctl(struct tty_struct *tty,
struct vt_stat __user *vtstat = up;
unsigned short state, mask;
- /* Review: FIXME: Console lock ? */
if (put_user(fg_console + 1, &vtstat->v_active))
ret = -EFAULT;
else {
state = 1; /* /dev/tty0 is always open */
+ console_lock();
for (i = 0, mask = 2; i < MAX_NR_CONSOLES && mask;
++i, mask <<= 1)
if (vt_in_use(i))
state |= mask;
+ console_unlock();
ret = put_user(state, &vtstat->v_state);
}
break;
@@ -661,10 +664,11 @@ int vt_ioctl(struct tty_struct *tty,
* Returns the first available (non-opened) console.
*/
case VT_OPENQRY:
- /* FIXME: locking ? - but then this is a stupid API */
+ console_lock();
for (i = 0; i < MAX_NR_CONSOLES; ++i)
if (!vt_in_use(i))
break;
+ console_unlock();
uival = i < MAX_NR_CONSOLES ? (i+1) : -1;
goto setint;
--
2.25.1
On 18. 03. 20, 23:38, Eric Biggers wrote:
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -43,9 +43,11 @@ bool vt_dont_switch;
>
> static inline bool vt_in_use(unsigned int i)
> {
> - extern struct tty_driver *console_driver;
> + const struct vc_data *vc = vc_cons[i].d;
>
> - return console_driver->ttys[i] && console_driver->ttys[i]->count;
> + WARN_CONSOLE_UNLOCKED();
> +
> + return vc && kref_read(&vc->port.kref) > 1;
> }
>
> static inline bool vt_busy(int i)
> @@ -643,15 +645,16 @@ int vt_ioctl(struct tty_struct *tty,
> struct vt_stat __user *vtstat = up;
> unsigned short state, mask;
>
> - /* Review: FIXME: Console lock ? */
> if (put_user(fg_console + 1, &vtstat->v_active))
> ret = -EFAULT;
> else {
> state = 1; /* /dev/tty0 is always open */
> + console_lock();
Could you comment on this one and the lock below why you added it here?
To me, it seems, we should rather introduce a vt alloc/dealloc lock
protecting cases like this, not console lock. But not now, some time
later. So a comment would help when/once we/I get into it...
The interface (ie. the ioctls) also look weird and racy. Both of them.
Like the "OK, I give you this number, but it might not be correct by
now." kind of thing.
This let me think, who could use this? The answer is many 8-/. openpt,
systemd, sysvinit, didn't check others.
Perhaps we should provide openvt -- analogy of openpty and deprecate
VT_OPENQRY?
With VT_GETSTATE, the situation is more complicated:
sysvinit uses VT_GETSTATE only if TIOCGDEV is not available, so
VT_GETSTATE is actually unneeded there.
systemd uses it to find the current console (vtstat->v_active) and
systemd-logind uses it for spawning autovt on free consoles. That sort
of makes sense...
> for (i = 0, mask = 2; i < MAX_NR_CONSOLES && mask;
> ++i, mask <<= 1)
> if (vt_in_use(i))
> state |= mask;
> + console_unlock();
> ret = put_user(state, &vtstat->v_state);
> }
> break;
> @@ -661,10 +664,11 @@ int vt_ioctl(struct tty_struct *tty,
> * Returns the first available (non-opened) console.
> */
> case VT_OPENQRY:
> - /* FIXME: locking ? - but then this is a stupid API */
> + console_lock();
> for (i = 0; i < MAX_NR_CONSOLES; ++i)
> if (!vt_in_use(i))
> break;
> + console_unlock();
> uival = i < MAX_NR_CONSOLES ? (i+1) : -1;
> goto setint;
>
>
thanks,
--
js
suse labs
On Fri, Mar 20, 2020 at 02:42:12PM +0100, Jiri Slaby wrote:
> On 18. 03. 20, 23:38, Eric Biggers wrote:
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -43,9 +43,11 @@ bool vt_dont_switch;
> >
> > static inline bool vt_in_use(unsigned int i)
> > {
> > - extern struct tty_driver *console_driver;
> > + const struct vc_data *vc = vc_cons[i].d;
> >
> > - return console_driver->ttys[i] && console_driver->ttys[i]->count;
> > + WARN_CONSOLE_UNLOCKED();
> > +
> > + return vc && kref_read(&vc->port.kref) > 1;
> > }
> >
> > static inline bool vt_busy(int i)
> > @@ -643,15 +645,16 @@ int vt_ioctl(struct tty_struct *tty,
> > struct vt_stat __user *vtstat = up;
> > unsigned short state, mask;
> >
> > - /* Review: FIXME: Console lock ? */
> > if (put_user(fg_console + 1, &vtstat->v_active))
> > ret = -EFAULT;
> > else {
> > state = 1; /* /dev/tty0 is always open */
> > + console_lock();
>
> Could you comment on this one and the lock below why you added it here?
>
> To me, it seems, we should rather introduce a vt alloc/dealloc lock
> protecting cases like this, not console lock. But not now, some time
> later. So a comment would help when/once we/I get into it...
I think the locking I added to VT_GETSTATE and VT_OPENQRY is pretty
self-explanatory: it's needed because they call vt_in_use() which now requires
console_lock. So I'm not sure what you'd like me to add there?
As for vt_in_use() itself, I already added WARN_CONSOLE_UNLOCKED() to it.
But I can add a comment to it too if it would be useful, like:
static inline bool vt_in_use(unsigned int i)
{
const struct vc_data *vc = vc_cons[i].d;
/*
* console_lock must be held to prevent the vc from being deallocated
* while we're checking whether it's in-use.
*/
WARN_CONSOLE_UNLOCKED();
return vc && kref_read(&vc->port.kref) > 1;
}
> The interface (ie. the ioctls) also look weird and racy. Both of them.
> Like the "OK, I give you this number, but it might not be correct by
> now." kind of thing.
>
> This let me think, who could use this? The answer is many 8-/. openpt,
> systemd, sysvinit, didn't check others.
>
> Perhaps we should provide openvt -- analogy of openpty and deprecate
> VT_OPENQRY?
>
> With VT_GETSTATE, the situation is more complicated:
> sysvinit uses VT_GETSTATE only if TIOCGDEV is not available, so
> VT_GETSTATE is actually unneeded there.
>
> systemd uses it to find the current console (vtstat->v_active) and
> systemd-logind uses it for spawning autovt on free consoles. That sort
> of makes sense...
>
Yes, these are bad APIs.
Once I did remove a buggy ioctl elsewhere in the kernel rather than fixing it.
But you have to be very, very confident that nothing is using it. That doesn't
seem to be the case for VT_GETSTATE and VT_OPENQRY as it's not hard to find code
using them. E.g. here's another user of both of them:
https://android.googlesource.com/platform/system/core/+/ccecf1425412beb2bc3bb38d470293fdc244d6f1/toolbox/setconsole.c
So we're probably stuck with them for now. If you'd like to explore adding a
better API and trying to get all users to use it, you're certainly welcome to.
But it would be orthogonal to fixing this bug.
- Eric
Fix VT_DISALLOCATE freeing an in-use virtual console, and fix a
use-after-free in vt_in_use().
Changed since v2:
- Implemented tty_port_operations::destruct().
- Added comments regarding vt_in_use() locking.
Changed since v1:
- Made the vc_data be freed via tty_port refcounting.
- Added patch to fix a use-after-free in vt_in_use().
Eric Biggers (2):
vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console
vt: vt_ioctl: fix use-after-free in vt_in_use()
drivers/tty/vt/vt.c | 23 ++++++++++++++++++++++-
drivers/tty/vt/vt_ioctl.c | 28 ++++++++++++++++------------
2 files changed, 38 insertions(+), 13 deletions(-)
--
2.25.2
On Sat, Mar 21, 2020 at 08:43:03PM -0700, Eric Biggers wrote:
> Fix VT_DISALLOCATE freeing an in-use virtual console, and fix a
> use-after-free in vt_in_use().
>
> Changed since v2:
> - Implemented tty_port_operations::destruct().
> - Added comments regarding vt_in_use() locking.
>
> Changed since v1:
> - Made the vc_data be freed via tty_port refcounting.
> - Added patch to fix a use-after-free in vt_in_use().
>
> Eric Biggers (2):
> vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console
> vt: vt_ioctl: fix use-after-free in vt_in_use()
>
> drivers/tty/vt/vt.c | 23 ++++++++++++++++++++++-
> drivers/tty/vt/vt_ioctl.c | 28 ++++++++++++++++------------
> 2 files changed, 38 insertions(+), 13 deletions(-)
Jiri, any objection to these?
thanks,
greg k-h