Re: [PATCH v2 2/2] vt: vt_ioctl: fix use-after-free in vt_in_use()

[Eric Biggers]

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