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

[Jiri Slaby <jslaby@suse.com>]

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