tree f7f636d3cae340400926531fb77f5d901826237e
parent 6c9e868886435703d3d2316c746aadcf5ccbb0a1
author Jann Horn <jannh@google.com> 1606958705 +0100
committer Dexter Rivera <riverade@google.com> 1609267213 +0000

tty: Fix ->session locking

commit c8bcd9c5be24fb9e6132e97da5a35e55a83e36b9 upstream.

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().

BUG=b/176281470
TEST=presubmit
RELEASE_NOTE=Fixed CVE-2020-29660
SOURCE=UPSTREAM(c8bcd9c5be24fb9e6132e97da5a35e55a83e36b9)

cos-patch: security-high
Cc: stable@kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-on: https://cos-review.googlesource.com/c/third_party/kernel/+/9621
Reviewed-by: Vaibhav Rustagi <vaibhavrustagi@google.com>
Tested-by: Cusky Presubmit Bot <presubmit@cos-infra-prod.iam.gserviceaccount.com>
Signed-off-by: Dexter Rivera <riverade@google.com>
Change-Id: I83d3547a5e7216c65ea1e2266d269300c444545d
Reviewed-on: https://cos-review.googlesource.com/c/third_party/kernel/+/10041
Reviewed-by: Robert Kolchmeyer <rkolchmeyer@google.com>
Reviewed-by: Roy Yang <royyang@google.com>