From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [REVIEW][PATCH 2/6] signal: Fail sigqueueinfo if si_signo != sig Date: Fri, 05 Oct 2018 08:27:41 +0200 Message-ID: <87h8i0di1u.fsf@xmission.com> References: <87h8idv6nw.fsf@xmission.com> <20180925171906.19683-2-ebiederm@xmission.com> <20181005060611.GA19061@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: <20181005060611.GA19061@gmail.com> (Andrei Vagin's message of "Thu, 4 Oct 2018 23:06:13 -0700") Sender: linux-kernel-owner@vger.kernel.org To: Andrei Vagin Cc: linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, linux-arch@vger.kernel.org, Oleg Nesterov , Linus Torvalds List-Id: linux-arch.vger.kernel.org Andrei Vagin writes: > On Tue, Sep 25, 2018 at 07:19:02PM +0200, Eric W. Biederman wrote: >> The kernel needs to validate that the contents of struct siginfo make >> sense as siginfo is copied into the kernel, so that the proper union >> members can be put in the appropriate locations. The field si_signo >> is a fundamental part of that validation. As such changing the >> contents of si_signo after the validation make no sense and can result >> in nonsense values in the kernel. > > Accoding to the man page, the user should not set si_signo, it has to be set > by kernel. > > $ man 2 rt_sigqueueinfo > > The uinfo argument specifies the data to accompany the signal. This > argument is a pointer to a structure of type siginfo_t, described in > sigaction(2) (and defined by including ). The caller > should set the following fields in this structure: > > si_code > This must be one of the SI_* codes in the Linux kernel source > file include/asm-generic/siginfo.h, with the restriction that > the code must be negative (i.e., cannot be SI_USER, which is > used by the kernel to indicate a signal sent by kill(2)) and > cannot (since Linux 2.6.39) be SI_TKILL (which is used by the > kernel to indicate a signal sent using tgkill(2)). > > si_pid This should be set to a process ID, typically the process ID of > the sender. > > si_uid This should be set to a user ID, typically the real user ID of > the sender. > > si_value > This field contains the user data to accompany the signal. For > more information, see the description of the last (union sigval) > argument of sigqueue(3). > > Internally, the kernel sets the si_signo field to the value specified > in sig, so that the receiver of the signal can also obtain the signal > number via that field. > >> >> As such simply fail if someone is silly enough to set si_signo out of >> sync with the signal number passed to sigqueueinfo. >> >> I don't expect a problem as glibc's sigqueue implementation sets >> "si_signo = sig" and CRIU just returns to the kernel what the kernel >> gave to it. >> >> If there is some application that calls sigqueueinfo directly that has >> a problem with this added sanity check we can revisit this when we see >> what kind of crazy that application is doing. > > > I already know two "applications" ;) > > https://github.com/torvalds/linux/blob/master/tools/testing/selftests/ptrace/peeksiginfo.c > https://github.com/checkpoint-restore/criu/blob/master/test/zdtm/static/sigpending.c > > Disclaimer: I'm the author of both of them. Fair enough. Then this counts as a regression. The setting in the kernel happens in an awkward place and I will see if it can be moved earlier. Eric >> Signed-off-by: "Eric W. Biederman" >> --- >> kernel/signal.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/signal.c b/kernel/signal.c >> index 7b49c31d3fdb..e445b0a63faa 100644 >> --- a/kernel/signal.c >> +++ b/kernel/signal.c >> @@ -3306,7 +3306,8 @@ static int do_rt_sigqueueinfo(pid_t pid, int sig, siginfo_t *info) >> (task_pid_vnr(current) != pid)) >> return -EPERM; >> >> - info->si_signo = sig; >> + if (info->si_signo != sig) >> + return -EINVAL; >> >> /* POSIX.1b doesn't mention process groups. */ >> return kill_proc_info(sig, info, pid); >> @@ -3354,7 +3355,8 @@ static int do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info) >> (task_pid_vnr(current) != pid)) >> return -EPERM; >> >> - info->si_signo = sig; >> + if (info->si_signo != sig) >> + return -EINVAL; >> >> return do_send_specific(tgid, pid, sig, info); >> } From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out02.mta.xmission.com ([166.70.13.232]:34690 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726732AbeJENZG (ORCPT ); Fri, 5 Oct 2018 09:25:06 -0400 From: ebiederm@xmission.com (Eric W. Biederman) References: <87h8idv6nw.fsf@xmission.com> <20180925171906.19683-2-ebiederm@xmission.com> <20181005060611.GA19061@gmail.com> Date: Fri, 05 Oct 2018 08:27:41 +0200 In-Reply-To: <20181005060611.GA19061@gmail.com> (Andrei Vagin's message of "Thu, 4 Oct 2018 23:06:13 -0700") Message-ID: <87h8i0di1u.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [REVIEW][PATCH 2/6] signal: Fail sigqueueinfo if si_signo != sig Sender: linux-arch-owner@vger.kernel.org List-ID: To: Andrei Vagin Cc: linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, linux-arch@vger.kernel.org, Oleg Nesterov , Linus Torvalds Message-ID: <20181005062741.ENutVMTnppv0s2f7nXo3xneyykvyKWy5Qh3jqPQl5ws@z> Andrei Vagin writes: > On Tue, Sep 25, 2018 at 07:19:02PM +0200, Eric W. Biederman wrote: >> The kernel needs to validate that the contents of struct siginfo make >> sense as siginfo is copied into the kernel, so that the proper union >> members can be put in the appropriate locations. The field si_signo >> is a fundamental part of that validation. As such changing the >> contents of si_signo after the validation make no sense and can result >> in nonsense values in the kernel. > > Accoding to the man page, the user should not set si_signo, it has to be set > by kernel. > > $ man 2 rt_sigqueueinfo > > The uinfo argument specifies the data to accompany the signal. This > argument is a pointer to a structure of type siginfo_t, described in > sigaction(2) (and defined by including ). The caller > should set the following fields in this structure: > > si_code > This must be one of the SI_* codes in the Linux kernel source > file include/asm-generic/siginfo.h, with the restriction that > the code must be negative (i.e., cannot be SI_USER, which is > used by the kernel to indicate a signal sent by kill(2)) and > cannot (since Linux 2.6.39) be SI_TKILL (which is used by the > kernel to indicate a signal sent using tgkill(2)). > > si_pid This should be set to a process ID, typically the process ID of > the sender. > > si_uid This should be set to a user ID, typically the real user ID of > the sender. > > si_value > This field contains the user data to accompany the signal. For > more information, see the description of the last (union sigval) > argument of sigqueue(3). > > Internally, the kernel sets the si_signo field to the value specified > in sig, so that the receiver of the signal can also obtain the signal > number via that field. > >> >> As such simply fail if someone is silly enough to set si_signo out of >> sync with the signal number passed to sigqueueinfo. >> >> I don't expect a problem as glibc's sigqueue implementation sets >> "si_signo = sig" and CRIU just returns to the kernel what the kernel >> gave to it. >> >> If there is some application that calls sigqueueinfo directly that has >> a problem with this added sanity check we can revisit this when we see >> what kind of crazy that application is doing. > > > I already know two "applications" ;) > > https://github.com/torvalds/linux/blob/master/tools/testing/selftests/ptrace/peeksiginfo.c > https://github.com/checkpoint-restore/criu/blob/master/test/zdtm/static/sigpending.c > > Disclaimer: I'm the author of both of them. Fair enough. Then this counts as a regression. The setting in the kernel happens in an awkward place and I will see if it can be moved earlier. Eric >> Signed-off-by: "Eric W. Biederman" >> --- >> kernel/signal.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/signal.c b/kernel/signal.c >> index 7b49c31d3fdb..e445b0a63faa 100644 >> --- a/kernel/signal.c >> +++ b/kernel/signal.c >> @@ -3306,7 +3306,8 @@ static int do_rt_sigqueueinfo(pid_t pid, int sig, siginfo_t *info) >> (task_pid_vnr(current) != pid)) >> return -EPERM; >> >> - info->si_signo = sig; >> + if (info->si_signo != sig) >> + return -EINVAL; >> >> /* POSIX.1b doesn't mention process groups. */ >> return kill_proc_info(sig, info, pid); >> @@ -3354,7 +3355,8 @@ static int do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info) >> (task_pid_vnr(current) != pid)) >> return -EPERM; >> >> - info->si_signo = sig; >> + if (info->si_signo != sig) >> + return -EINVAL; >> >> return do_send_specific(tgid, pid, sig, info); >> }