| 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
| ||
|
Date: Tue, 30 Mar 2010 15:57:49 -0700
From: Greg KH <gregkh@...e.de>
To: linux-kernel@...r.kernel.org, stable@...nel.org
Cc: stable-review@...nel.org, torvalds@...ux-foundation.org,
akpm@...ux-foundation.org, alan@...rguk.ukuu.org.uk,
Greg Kroah-Hartman <gregkh@...e.de>,
Américo Wang <xiyou.wangcong@...il.com>
Subject: [47/89] Fix race in tty_fasync() properly
2.6.31-stable review patch. If anyone has any objections, please let us know.
------------------
From: Linus Torvalds <torvalds@...ux-foundation.org>
commit 80e1e823989ec44d8e35bdfddadbddcffec90424 upstream.
This reverts commit 703625118069 ("tty: fix race in tty_fasync") and
commit b04da8bfdfbb ("fnctl: f_modown should call write_lock_irqsave/
restore") that tried to fix up some of the fallout but was incomplete.
It turns out that we really cannot hold 'tty->ctrl_lock' over calling
__f_setown, because not only did that cause problems with interrupt
disables (which the second commit fixed), it also causes a potential
ABBA deadlock due to lock ordering.
Thanks to Tetsuo Handa for following up on the issue, and running
lockdep to show the problem. It goes roughly like this:
- f_getown gets filp->f_owner.lock for reading without interrupts
disabled, so an interrupt that happens while that lock is held can
cause a lockdep chain from f_owner.lock -> sighand->siglock.
- at the same time, the tty->ctrl_lock -> f_owner.lock chain that
commit 703625118069 introduced, together with the pre-existing
sighand->siglock -> tty->ctrl_lock chain means that we have a lock
dependency the other way too.
So instead of extending tty->ctrl_lock over the whole __f_setown() call,
we now just take a reference to the 'pid' structure while holding the
lock, and then release it after having done the __f_setown. That still
guarantees that 'struct pid' won't go away from under us, which is all
we really ever needed.
Reported-and-tested-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Acked-by: Greg Kroah-Hartman <gregkh@...e.de>
Acked-by: Américo Wang <xiyou.wangcong@...il.com>
Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@...e.de>
---
drivers/char/tty_io.c | 4 +++-
fs/fcntl.c | 6 ++----
2 files changed, 5 insertions(+), 5 deletions(-)
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1912,8 +1912,10 @@ static int tty_fasync(int fd, struct fil
pid = task_pid(current);
type = PIDTYPE_PID;
}
- retval = __f_setown(filp, pid, type, 0);
+ get_pid(pid);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+ retval = __f_setown(filp, pid, type, 0);
+ put_pid(pid);
if (retval)
goto out;
} else {
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -199,9 +199,7 @@ static int setfl(int fd, struct file * f
static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
int force)
{
- unsigned long flags;
-
- write_lock_irqsave(&filp->f_owner.lock, flags);
+ write_lock_irq(&filp->f_owner.lock);
if (force || !filp->f_owner.pid) {
put_pid(filp->f_owner.pid);
filp->f_owner.pid = get_pid(pid);
@@ -213,7 +211,7 @@ static void f_modown(struct file *filp,
filp->f_owner.euid = cred->euid;
}
}
- write_unlock_irqrestore(&filp->f_owner.lock, flags);
+ write_unlock_irq(&filp->f_owner.lock);
}
int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists