• Jann Horn's avatar
    tty: Fix ->session locking · c8bcd9c5
    Jann Horn authored
    Currently, locking of ->session is very inconsistent; most places
    protect it using the legacy tty mutex, but disassociate_ctty(),
    __do_SAK(), tiocspgrp() and tiocgsid() don't.
    Two of the writers hold the ctrl_lock (because they already need it for
    ->pgrp), but __proc_set_tty() doesn't do that yet.
    
    On a PREEMPT=y system, an unprivileged user can theoretically abuse
    this broken locking to read 4 bytes of freed memory via TIOCGSID if
    tiocgsid() is preempted long enough at the right point. (Other things
    might also go wrong, especially if root-only ioctls are involved; I'm
    not sure about that.)
    
    Change the locking on ->session such that:
    
     - tty_lock() is held by all writers: By making disassociate_ctty()
       hold it. This should be fine because the same lock can already be
       taken through the call to tty_vhangup_session().
       The tricky part is that we need to shorten the area covered by
       siglock to be able to take tty_lock() without ugly retry logic; as
       far as I can tell, this should be fine, since nothing in the
       signal_struct is touched in the `if (tty)` branch.
     - ctrl_lock is held by all writers: By changing __proc_set_tty() to
       hold the lock a little longer.
     - All readers that aren't holding tty_lock() hold ctrl_lock: By
       adding locking to tiocgsid() and __do_SAK(), and expanding the area
       covered by ctrl_lock in tiocspgrp().
    
    Cc: stable@kernel.org
    Signed-off-by: default avatarJann Horn <jannh@google.com>
    Reviewed-by: default avatarJiri Slaby <jirislaby@kernel.org>
    Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    c8bcd9c5
tty_io.c 85.1 KB