From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_PASS,URIBL_BLOCKED,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 93008C43381 for ; Sat, 30 Mar 2019 01:34:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5AA072184C for ; Sat, 30 Mar 2019 01:34:28 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=brauner.io header.i=@brauner.io header.b="MsdB1jhv" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732206AbfC3Be0 (ORCPT ); Fri, 29 Mar 2019 21:34:26 -0400 Received: from mail-ed1-f67.google.com ([209.85.208.67]:45119 "EHLO mail-ed1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731060AbfC3BeV (ORCPT ); Fri, 29 Mar 2019 21:34:21 -0400 Received: by mail-ed1-f67.google.com with SMTP id m16so3525493edd.12 for ; Fri, 29 Mar 2019 18:34:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=brauner.io; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=tBykamkGXGhl6g5OEQcE2XhD7B931pY9hJ+thiyv0u4=; b=MsdB1jhvLCntID2DE9REDtIIJU1Brw/9Qc+lSTdpFEOsOX9uLJMjbVyVtUt2duARuo iZb2mHAwFkuvftXQHDSVgZA94WyAZ3F6xDLymkjZ+CcPo2DzHwXZE6OkwIBgx6nq+yV7 YtCuuWVrY40ZYeBNRLgvS5fqR/oAwwQ0tO+/H+8GGE049SzjCzGWs2tOisbBKRjZBjmS aqY36PH/soM3xk7h7mioN6oeutqn8e4BKFd9VU1qY4yrJmBIgrSp9M5MtFcHgCILiRuA XivrtPKz9ST38rPnLR3VHGSZMEteg1nOkqBhGLbS+unIotIN1qgxhuLAvrrpeZjfjjY+ WPvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=tBykamkGXGhl6g5OEQcE2XhD7B931pY9hJ+thiyv0u4=; b=GedKERigQugKw5VFxanA3abpu50KXc9wnFHe7+HYxQHssqxBDA7q69CpqoCCkiCMNe C4Kr6AIJwyA8ptMV4EMjwdvqjPSbY7KcofR+gYGczW1D0JqwcSgWNULhtREgyYr4CbF8 qA+8rg2gUixHE/xZB6VgsdWQfu/qqgkzB5wSWTsXaLtUA6fs1UC63g0VzNixxGjXA5vV CQssZ1mbi7hPXehC6xFbXfZnydM5RhrI+MjGO587UfY3LfeYKUM3vP5KfHMbuG/mM3dF h8T3x5bcVI6YZ1pSv5d0bkAqt0moOMS4yE8DVdwQva416agEFtUIIuGA2lw6dR4A2hW+ 34zA== X-Gm-Message-State: APjAAAXxNExAmd5EJielzkyfH4Bjo8E2NV+3SF43pqbR1fWFzawM1yJX GvWimBp566iHF5k89Y+capUtzg== X-Google-Smtp-Source: APXvYqzwZejrsokjFgwpyZOtXLYAEVmrhJefNsKFWTLPa531h+rglKpofKM3PFSW2EFos/T+n/0lPA== X-Received: by 2002:a50:95fa:: with SMTP id x55mr32103298eda.49.1553909659519; Fri, 29 Mar 2019 18:34:19 -0700 (PDT) Received: from brauner.io ([2a02:8109:b6bf:d24a:b136:35b0:7c8c:280a]) by smtp.gmail.com with ESMTPSA id d37sm1086791ede.79.2019.03.29.18.34.18 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Fri, 29 Mar 2019 18:34:19 -0700 (PDT) Date: Sat, 30 Mar 2019 02:34:17 +0100 From: Christian Brauner To: Jann Horn Cc: Andy Lutomirski , David Howells , "Serge E. Hallyn" , Linux API , kernel list , Arnd Bergmann , "Eric W. Biederman" , Konstantin Khlebnikov , Kees Cook , Alexey Dobriyan , Thomas Gleixner , Michael Kerrisk-manpages , Jonathan Kowalski , "Dmitry V. Levin" , Andrew Morton , Oleg Nesterov , Nagarathnam Muthusamy , Aleksa Sarai , Al Viro , "Joel Fernandes (Google)" , Daniel Colascione , Florian Weimer Subject: Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds Message-ID: <20190330013416.vnidfkjbsxxhzslm@brauner.io> References: <20190329155425.26059-1-christian@brauner.io> <20190329155425.26059-5-christian@brauner.io> <20190330012229.yt3hecmgaj2r6vp7@brauner.io> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190330012229.yt3hecmgaj2r6vp7@brauner.io> User-Agent: NeoMutt/20180716 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Mar 30, 2019 at 02:22:29AM +0100, Christian Brauner wrote: > On Sat, Mar 30, 2019 at 02:06:34AM +0100, Jann Horn wrote: > > On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner wrote: > > > With the addition of pidfd_open() it is possible for users to reference a > > > specific thread by doing: > > > > > > int pidfd = pidfd_open(, 0); > > > > > > This means we can extend pidfd_send_signal() to signal a specific thread. > > > As promised in the commit for pidfd_send_signal() [1] the extension is > > > based on a flag argument, i.e. the scope of the signal delivery is based on > > > the flag argument, not on the type of file descriptor. > > > To this end the flag PIDFD_SIGNAL_TID is added. With this change we now > > > cover most of the functionality of all the other signal sending functions > > > combined: > > [...] > > > diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h > > > index d6c7c0701997..b72f0ef84fe5 100644 > > > --- a/include/uapi/linux/wait.h > > > +++ b/include/uapi/linux/wait.h > > [...] > > > +/* Flags to pass to pidfd_send_signal */ > > > +#define PIDFD_SIGNAL_TID 1 /* Send signal to specific thread */ > > > > nit: s/1/1U/; the flags argument is an `unsigned int` > > Will change. > > > > > > #endif /* _UAPI_LINUX_WAIT_H */ > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > index eb97d0cc6ef7..9f93da85b2b9 100644 > > > --- a/kernel/signal.c > > > +++ b/kernel/signal.c > > [...] > > > +static int pidfd_send_signal_specific(struct pid *pid, int sig, > > > + struct kernel_siginfo *info) > > > +{ > > > + struct task_struct *p; > > > + int error = -ESRCH; > > > + > > > + rcu_read_lock(); > > > + p = pid_task(pid, PIDTYPE_PID); > > > + if (p) > > > + error = __do_send_specific(p, sig, info); > > > + rcu_read_unlock(); > > > + > > > + return error; > > > +} > > > + > > > /** > > > - * sys_pidfd_send_signal - send a signal to a process through a task file > > > - * descriptor > > > + * sys_pidfd_send_signal - send a signal to a process through a pidfd > > > + > > > * @pidfd: the file descriptor of the process > > > * @sig: signal to be sent > > > * @info: the signal info > > > * @flags: future flags to be passed > > > > nit: comment is outdated, it isn't "future flags" anymore > > Will remove. > > > > > [...] > > > + * rt_tgsigqueueinfo(, , , ) > > > + * - pidfd_send_signal(, , , PIDFD_SIGNAL_TID); > > > + * which is equivalent to > > > + * rt_tgsigqueueinfo(, , , ) > > > + * > > > * In order to extend the syscall to threads and process groups the @flags > > > * argument should be used. In essence, the @flags argument will determine > > > * what is signaled and not the file descriptor itself. Put in other words, > > > > nit: again, outdated comment about @flags > > Will update. > > > > > [...] > > > @@ -3626,43 +3695,16 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > > prepare_kill_siginfo(sig, &kinfo); > > > } > > > > > > - ret = kill_pid_info(sig, &kinfo, pid); > > > + if (flags & PIDFD_SIGNAL_TID) > > > + ret = pidfd_send_signal_specific(pid, sig, &kinfo); > > > + else > > > + ret = kill_pid_info(sig, &kinfo, pid); > > > > nit: maybe give pidfd_send_signal_specific() and kill_pid_info() the > > same signatures, since they perform similar operations with the same > > argument types? > > Yes, let's do > pidfd_send_signal_specific.(pid, sig, &kinfo); > kill_pid_info..............(pid, sig, &kinfo); > > so it matches the argument order of the syscalls itself too. Strike that. We should do: pidfd_send_signal_specific.(sig, &kinfo, pid); kill_pid_info..............(sig, &kinfo, pid); because kill_pid_info() is called in multiple places so we would needlessly shovle code around. > > > > > Something that was already kinda weird in the existing code, but is > > getting worse with TIDs is the handling of SI_USER with siginfo. > > Right, that's what we discussed earlier. > > > Copying context lines from above here: > > > > if (info) { > > ret = copy_siginfo_from_user_any(&kinfo, info); > > if (unlikely(ret)) > > goto err; > > ret = -EINVAL; > > if (unlikely(sig != kinfo.si_signo)) > > goto err; > > if ((task_pid(current) != pid) && > > (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) { > > /* Only allow sending arbitrary signals to yourself. */ > > ret = -EPERM; > > if (kinfo.si_code != SI_USER) > > goto err; > > /* Turn this into a regular kill signal. */ > > prepare_kill_siginfo(sig, &kinfo); > > } > > } else { > > prepare_kill_siginfo(sig, &kinfo); > > } > > > > So for signals to PIDs, the rule is that if you send siginfo with > > SI_USER to yourself, the siginfo is preserved; otherwise the kernel > > silently clobbers it. That's already kind of weird - silent behavior > > Clobbers as in "silently replaces it whatever it seems fit? > > > difference depending on a security check. But now, for signals to > > threads, I think the result is going to be that signalling the thread > > group leader preserves information, and signalling any other thread > > clobbers it? If so, that seems bad. > > > > do_rt_sigqueueinfo() seems to have the same issue, from a glance - but > > there, at least the error case is just a -EPERM, not a silent behavior > > difference. > > > > Would it make sense to refuse sending siginfo with SI_USER to > > non-current? If you actually want to send a normal SI_USER signal, you > > Yeah. > > > can use info==NULL, right? That should create wrongness parity with > > do_rt_sigqueueinfo(). > > So you'd just do (just doing it non-elegantly rn): > if ((task_pid(current) != pid) && > (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) { > ret = -EPERM; > goto err; > } > > > To improve things further, I guess you'd have to move the comparison > > against current into pidfd_send_signal_specific(), or move the task > > lookup out of it, or something like that? > > Looks like a sane suggestion to me. Would you care to send a patch for > that? This is clearly a bugfix suitable for 5.1 so I'd rather not wait > until 5.2. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Brauner Subject: Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds Date: Sat, 30 Mar 2019 02:34:17 +0100 Message-ID: <20190330013416.vnidfkjbsxxhzslm@brauner.io> References: <20190329155425.26059-1-christian@brauner.io> <20190329155425.26059-5-christian@brauner.io> <20190330012229.yt3hecmgaj2r6vp7@brauner.io> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Content-Disposition: inline In-Reply-To: <20190330012229.yt3hecmgaj2r6vp7@brauner.io> Sender: linux-kernel-owner@vger.kernel.org To: Jann Horn Cc: Andy Lutomirski , David Howells , "Serge E. Hallyn" , Linux API , kernel list , Arnd Bergmann , "Eric W. Biederman" , Konstantin Khlebnikov , Kees Cook , Alexey Dobriyan , Thomas Gleixner , Michael Kerrisk-manpages , Jonathan Kowalski , "Dmitry V. Levin" , Andrew Morton , Oleg Nesterov , Nagarathnam Muthusamy , Aleksa Sarai , Al Viro List-Id: linux-api@vger.kernel.org On Sat, Mar 30, 2019 at 02:22:29AM +0100, Christian Brauner wrote: > On Sat, Mar 30, 2019 at 02:06:34AM +0100, Jann Horn wrote: > > On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner wrote: > > > With the addition of pidfd_open() it is possible for users to reference a > > > specific thread by doing: > > > > > > int pidfd = pidfd_open(, 0); > > > > > > This means we can extend pidfd_send_signal() to signal a specific thread. > > > As promised in the commit for pidfd_send_signal() [1] the extension is > > > based on a flag argument, i.e. the scope of the signal delivery is based on > > > the flag argument, not on the type of file descriptor. > > > To this end the flag PIDFD_SIGNAL_TID is added. With this change we now > > > cover most of the functionality of all the other signal sending functions > > > combined: > > [...] > > > diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h > > > index d6c7c0701997..b72f0ef84fe5 100644 > > > --- a/include/uapi/linux/wait.h > > > +++ b/include/uapi/linux/wait.h > > [...] > > > +/* Flags to pass to pidfd_send_signal */ > > > +#define PIDFD_SIGNAL_TID 1 /* Send signal to specific thread */ > > > > nit: s/1/1U/; the flags argument is an `unsigned int` > > Will change. > > > > > > #endif /* _UAPI_LINUX_WAIT_H */ > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > index eb97d0cc6ef7..9f93da85b2b9 100644 > > > --- a/kernel/signal.c > > > +++ b/kernel/signal.c > > [...] > > > +static int pidfd_send_signal_specific(struct pid *pid, int sig, > > > + struct kernel_siginfo *info) > > > +{ > > > + struct task_struct *p; > > > + int error = -ESRCH; > > > + > > > + rcu_read_lock(); > > > + p = pid_task(pid, PIDTYPE_PID); > > > + if (p) > > > + error = __do_send_specific(p, sig, info); > > > + rcu_read_unlock(); > > > + > > > + return error; > > > +} > > > + > > > /** > > > - * sys_pidfd_send_signal - send a signal to a process through a task file > > > - * descriptor > > > + * sys_pidfd_send_signal - send a signal to a process through a pidfd > > > + > > > * @pidfd: the file descriptor of the process > > > * @sig: signal to be sent > > > * @info: the signal info > > > * @flags: future flags to be passed > > > > nit: comment is outdated, it isn't "future flags" anymore > > Will remove. > > > > > [...] > > > + * rt_tgsigqueueinfo(, , , ) > > > + * - pidfd_send_signal(, , , PIDFD_SIGNAL_TID); > > > + * which is equivalent to > > > + * rt_tgsigqueueinfo(, , , ) > > > + * > > > * In order to extend the syscall to threads and process groups the @flags > > > * argument should be used. In essence, the @flags argument will determine > > > * what is signaled and not the file descriptor itself. Put in other words, > > > > nit: again, outdated comment about @flags > > Will update. > > > > > [...] > > > @@ -3626,43 +3695,16 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > > prepare_kill_siginfo(sig, &kinfo); > > > } > > > > > > - ret = kill_pid_info(sig, &kinfo, pid); > > > + if (flags & PIDFD_SIGNAL_TID) > > > + ret = pidfd_send_signal_specific(pid, sig, &kinfo); > > > + else > > > + ret = kill_pid_info(sig, &kinfo, pid); > > > > nit: maybe give pidfd_send_signal_specific() and kill_pid_info() the > > same signatures, since they perform similar operations with the same > > argument types? > > Yes, let's do > pidfd_send_signal_specific.(pid, sig, &kinfo); > kill_pid_info..............(pid, sig, &kinfo); > > so it matches the argument order of the syscalls itself too. Strike that. We should do: pidfd_send_signal_specific.(sig, &kinfo, pid); kill_pid_info..............(sig, &kinfo, pid); because kill_pid_info() is called in multiple places so we would needlessly shovle code around. > > > > > Something that was already kinda weird in the existing code, but is > > getting worse with TIDs is the handling of SI_USER with siginfo. > > Right, that's what we discussed earlier. > > > Copying context lines from above here: > > > > if (info) { > > ret = copy_siginfo_from_user_any(&kinfo, info); > > if (unlikely(ret)) > > goto err; > > ret = -EINVAL; > > if (unlikely(sig != kinfo.si_signo)) > > goto err; > > if ((task_pid(current) != pid) && > > (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) { > > /* Only allow sending arbitrary signals to yourself. */ > > ret = -EPERM; > > if (kinfo.si_code != SI_USER) > > goto err; > > /* Turn this into a regular kill signal. */ > > prepare_kill_siginfo(sig, &kinfo); > > } > > } else { > > prepare_kill_siginfo(sig, &kinfo); > > } > > > > So for signals to PIDs, the rule is that if you send siginfo with > > SI_USER to yourself, the siginfo is preserved; otherwise the kernel > > silently clobbers it. That's already kind of weird - silent behavior > > Clobbers as in "silently replaces it whatever it seems fit? > > > difference depending on a security check. But now, for signals to > > threads, I think the result is going to be that signalling the thread > > group leader preserves information, and signalling any other thread > > clobbers it? If so, that seems bad. > > > > do_rt_sigqueueinfo() seems to have the same issue, from a glance - but > > there, at least the error case is just a -EPERM, not a silent behavior > > difference. > > > > Would it make sense to refuse sending siginfo with SI_USER to > > non-current? If you actually want to send a normal SI_USER signal, you > > Yeah. > > > can use info==NULL, right? That should create wrongness parity with > > do_rt_sigqueueinfo(). > > So you'd just do (just doing it non-elegantly rn): > if ((task_pid(current) != pid) && > (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) { > ret = -EPERM; > goto err; > } > > > To improve things further, I guess you'd have to move the comparison > > against current into pidfd_send_signal_specific(), or move the task > > lookup out of it, or something like that? > > Looks like a sane suggestion to me. Would you care to send a patch for > that? This is clearly a bugfix suitable for 5.1 so I'd rather not wait > until 5.2.