Re: [PATCH 0/8] Fix unsafe uart port access

[Peter Hurley]

On 12/12/2015 04:36 PM, Peter Hurley wrote:
> Hi Greg,
> 
> The serial core has always intended to allow uart drivers to detach and
> unload, even while ttys are open and running. Since the serial core is
> the actual tty driver, it acts as a proxy for the uart driver, which
> allows the uart driver to remove the port (which hangs up the tty) and
> then unload.
> 
> However, all of this is broken.
> 
> Firstly, there is no mechanism for blocking the uart port removal until
> current operations are complete. The tty core does not, and never has,
> serialized ioctl() or any other tty operation other than open/close
> with hangup.
> 
> Secondly, uart_register_driver() directly binds data from the uart driver
> with the tty core, rather than providing proxy copies _without taking
> module references to the uart driver_. This produces some spectacular
> results if the tty is in-use when the uart driver unloads.
> 
> Thirdly, uart_unregister_driver() immediately destroys the tty port
> despite that it may be in use, and will continue to be for the
> lifetime of the tty, which is unbounded (userspace may retain open
> ttys forever).
> 
> This series addresses the first problem of ensuring safe uart port
> dereferencing with concurrent uart port removal. Two distinct
> mechanisms are used to provide the necessary safe dereference: the
> tty_port mutex and RCU.
> 
> By serializing the uart port detach (ie, reset to NULL) with the
> tty_port mutex, existing sections of the serial core that already
> hold the port mutex are guaranteed the uart port detach will not
> be concurrent, and so can simply test for NULL value before first
> dereference.
> 
> Most of the existing serial core that holds the port mutex is
> ioctl-related and so can return -EIO as if the tty has been hungup
> (which it has been).
> 
> Other portions of the serial core that sleep (eg. uart_wait_until_sent())
> also now claim the port mutex to prevent concurrent uart port detach,
> and thus protect the reference. The port mutex is dropped for the
> actual sleep, retaken on wakeup and the uart port is re-checked
> for NULL.
> 
> For portions of the serial core that don't sleep, RCU is used to
> defer actual uart port teardown after detach (with synchronize_rcu())
> until the current holders of rcu_read_lock() are complete.
> 
> The xmit buffer operations that don't modify the buffer indexes --
> uart_write_room() and uart_chars_in_buffer() -- just report the state
> of the xmit buffer anyway if the uart port has been removed, despite
> that the xmit buffer is no longer lockable (the lock is in the
> uart_port that is now gone).
> 
> Xmit buffer operations that do modify the buffer indexes are aborted.
> 
> Extra care is taken with the close/hangup/shutdown paths since this
> must continue to perform other work even if the uart port has been
> removed (for example, if the uart port was /dev/console and so will
> only be closed when the file descriptor is released).
> 
> I do have a plan for the second and third problems but this series
> does not implement those.
> 
> Regards,
> 
> Peter Hurley (8):
>   serial: core: Fold __uart_put_char() into caller
>   serial: core: Fold do_uart_get_info() into caller
>   serial: core: Use tty->index for port # in debug messages

Hi Greg,

I split the first three patches above and submitted those
in the "Misc serial cleanups" v2 series.

For the remaining patches below, I need to rework how to
guarantee the lifespan of uart port during throttle/unthrottle,
and then rebase the following patches.

Regards
Peter Hurley

>   serial: core: Take port mutex for send_xchar() tty method
>   serial: core: Expand port mutex section in uart_poll_init()
>   serial: core: Prevent unsafe uart port access, part 1
>   serial: core: Prevent unsafe uart port access, part 2
>   serial: core: Prevent unsafe uart port access, part 3
> 
>  drivers/tty/serial/8250/8250_port.c |   6 +-
>  drivers/tty/serial/serial_core.c    | 547 +++++++++++++++++++++++-------------
>  2 files changed, 355 insertions(+), 198 deletions(-)
>