linux-kernel - Re: strange interaction between fuse + pidns

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: <YsyHMVLuT5U6mm+I@netflix>
Date:   Mon, 11 Jul 2022 14:25:21 -0600
From:   Tycho Andersen <tycho@...ho.pizza>
To:     Miklos Szeredi <miklos@...redi.hu>,
        Eric Biederman <ebiederm@...ssion.com>
Cc:     Christian Brauner <brauner@...nel.org>,
        fuse-devel <fuse-devel@...ts.sourceforge.net>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: strange interaction between fuse + pidns

Hi all,

On Mon, Jul 11, 2022 at 03:59:15PM +0200, Miklos Szeredi wrote:
> On Mon, 11 Jul 2022 at 12:35, Miklos Szeredi <miklos@...redi.hu> wrote:
> >
> > Can you try the attached untested patch?
> 
> Updated patch to avoid use after free on req->args.
> 
> Still mostly untested.

Thanks, when I applied your patch, I still ended up with tasks stuck
waiting with a SIGKILL pending. So I looked into that and came up with
the patch below. With both your patch and mine, my testcase exits
cleanly.

Eric (or Christian, or anyone), can you comment on the patch below? I
have no idea what this will break. Maybe instead a better approach is
some additional special case in __send_signal_locked()?

Tycho

>From b7ea26adcf3546be5745063cc86658acb5ed37e9 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@...ho.pizza>
Date: Mon, 11 Jul 2022 11:26:58 -0600
Subject: [PATCH] sched: __fatal_signal_pending() should also check shared
 signals

The wait_* code uses signal_pending_state() to test whether a thread has
been interrupted, which ultimately uses __fatal_signal_pending() to detect
if there is a fatal signal.

When a pid ns dies, in zap_pid_ns_processes() it does:

    group_send_sig_info(SIGKILL, SEND_SIG_PRIV, task, PIDTYPE_MAX);

for all the tasks in the pid ns. That calls through:

    group_send_sig_info() ->
      do_send_sig_info() ->
        send_signal_locked() ->
          __send_signal_locked()

which does:

    pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;

which puts sigkill in the set of shared signals, but not the individual
pending ones. If tasks are stuck in a killable wait (e.g. a fuse flush
operation), they won't see this shared signal, and will hang forever, since
TIF_SIGPENDING is set, but the fatal signal can't be detected.

Signed-off-by: Tycho Andersen <tycho@...ho.pizza>
---
 include/linux/sched/signal.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index cafbe03eed01..a033ccb0a729 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -402,7 +402,8 @@ static inline int signal_pending(struct task_struct *p)

 static inline int __fatal_signal_pending(struct task_struct *p)
 {
-	return unlikely(sigismember(&p->pending.signal, SIGKILL));
+	return unlikely(sigismember(&p->pending.signal, SIGKILL) ||
+			sigismember(&p->signal->shared_pending.signal, SIGKILL));
 }

 static inline int fatal_signal_pending(struct task_struct *p)

base-commit: 32346491ddf24599decca06190ebca03ff9de7f8
-- 
2.34.1