linux-kernel - Re: [patch 1/3] tty: resolve tty contention between kernel and user space

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170709115139.GB14769@kroah.com>
Date:   Sun, 9 Jul 2017 13:51:39 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Okash Khawaja <okash.khawaja@...il.com>
Cc:     Jiri Slaby <jslaby@...e.com>,
        Samuel Thibault <samuel.thibault@...-lyon.org>,
        Alan Cox <gnomes@...rguk.ukuu.org.uk>,
        linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
        Kirk Reiser <kirk@...sers.ca>, speakup@...ux-speakup.org,
        Chris Brannon <chris@...-brannons.com>
Subject: Re: [patch 1/3] tty: resolve tty contention between kernel and user
 space

On Sun, Jul 09, 2017 at 12:41:54PM +0100, Okash Khawaja wrote:
> The commit 12e84c71b7d4ee (tty: export tty_open_by_driver) exports
> tty_open_by_device to allow tty to be opened from inside kernel which
> works fine except that it doesn't handle contention with user space or
> another kernel-space open of the same tty. For example, opening a tty
> from user space while it is kernel opened results in failure and a
> kernel log message about mismatch between tty->count and tty's file
> open count.
> 
> This patch makes kernel access to tty exclusive, so that if a user
> process or kernel opens a kernel opened tty, it gets -EBUSY. It does
> this by adding TTY_KOPENED flag to tty->flags. When this flag is set,
> tty_open_by_driver returns -EBUSY. Instead of overlaoding
> tty_open_by_driver for both kernel and user space, this
> patch creates a separate function tty_kopen which closely follows
> tty_open_by_driver.
> 
> Returning -EBUSY on tty open is a change in the interface. I have
> tested this with minicom, picocom and commands like "echo foo >
> /dev/ttyS0". They all correctly report "Device or resource busy" when
> the tty is already kernel opened.
> 
> Signed-off-by: Okash Khawaja <okash.khawaja@...il.com>
> 
> ---
>  drivers/tty/tty_io.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/tty.h  |    4 +++
>  2 files changed, 58 insertions(+)
> 
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1786,6 +1786,54 @@ static struct tty_driver *tty_lookup_dri
>  }
>  
>  /**
> + *	tty_kopen	-	open a tty device for kernel
> + *	@device: dev_t of device to open
> + *
> + *	Opens tty exclusively for kernel. Performs the driver lookup,
> + *	makes sure it's not already opened and performs the first-time
> + *	tty initialization.
> + *
> + *	Returns the locked initialized &tty_struct
> + *
> + *	Claims the global tty_mutex to serialize:
> + *	  - concurrent first-time tty initialization
> + *	  - concurrent tty driver removal w/ lookup
> + *	  - concurrent tty removal from driver table
> + */
> +struct tty_struct *tty_kopen(dev_t device)
> +{
> +	struct tty_struct *tty;
> +	struct tty_driver *driver = NULL;
> +	int index = -1;
> +
> +	mutex_lock(&tty_mutex);
> +	driver = tty_lookup_driver(device, NULL, &index);
> +	if (IS_ERR(driver)) {
> +		mutex_unlock(&tty_mutex);
> +		return ERR_CAST(driver);
> +	}
> +
> +	/* check whether we're reopening an existing tty */
> +	tty = tty_driver_lookup_tty(driver, NULL, index);
> +	if (IS_ERR(tty))
> +		goto out;
> +
> +	if (tty) {
> +		/* drop kref from tty_driver_lookup_tty() */
> +		tty_kref_put(tty);
> +		tty = ERR_PTR(-EBUSY);
> +	} else { /* tty_init_dev returns tty with the tty_lock held */
> +		tty = tty_init_dev(driver, index);
> +		set_bit(TTY_KOPENED, &tty->flags);
> +	}
> +out:
> +	mutex_unlock(&tty_mutex);
> +	tty_driver_kref_put(driver);
> +	return tty;
> +}
> +EXPORT_SYMBOL_GPL(tty_kopen);
> +
> +/**
>   *	tty_open_by_driver	-	open a tty device
>   *	@device: dev_t of device to open
>   *	@inode: inode of device file
> @@ -1824,6 +1872,12 @@ struct tty_struct *tty_open_by_driver(de
>  	}
>  
>  	if (tty) {
> +		if (test_bit(TTY_KOPENED, &tty->flags)) {
> +			tty_kref_put(tty);
> +			mutex_unlock(&tty_mutex);
> +			tty = ERR_PTR(-EBUSY);
> +			goto out;
> +		}
>  		mutex_unlock(&tty_mutex);
>  		retval = tty_lock_interruptible(tty);
>  		tty_kref_put(tty);  /* drop kref from tty_driver_lookup_tty() */
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -362,6 +362,7 @@ struct tty_file_private {
>  #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
>  #define TTY_HUPPED 		18	/* Post driver->hangup() */
>  #define TTY_LDISC_HALTED	22	/* Line discipline is halted */
> +#define TTY_KOPENED		23	/* TTY exclusively opened by kernel */
>  
>  /* Values for tty->flow_change */
>  #define TTY_THROTTLE_SAFE 1
> @@ -401,6 +402,7 @@ extern int __init tty_init(void);
>  extern const char *tty_name(const struct tty_struct *tty);
>  extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
>  		struct file *filp);
> +extern struct tty_struct *tty_kopen(dev_t device);
>  extern int tty_dev_name_to_number(const char *name, dev_t *number);
>  #else
>  static inline void tty_kref_put(struct tty_struct *tty)
> @@ -425,6 +427,8 @@ static inline const char *tty_name(const
>  static inline struct tty_struct *tty_open_by_driver(dev_t device,
>  		struct inode *inode, struct file *filp)
>  { return NULL; }
> +static inline struct tty_struct *tty_kopen(dev_t device)
> +{ return NULL; }

Like I said in response to patch 2, maybe this should return an error?

thanks,

greg k-h