From: ebiederm@xmission.com (Eric W. Biederman)
To: Andrei Vagin <avagin@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-arch@vger.kernel.org, Oleg Nesterov <oleg@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [REVIEW][PATCH 2/6] signal: Fail sigqueueinfo if si_signo != sig
Date: Fri, 05 Oct 2018 08:27:41 +0200 [thread overview]
Message-ID: <87h8i0di1u.fsf@xmission.com> (raw)
In-Reply-To: <20181005060611.GA19061@gmail.com> (Andrei Vagin's message of "Thu, 4 Oct 2018 23:06:13 -0700")
Andrei Vagin <avagin@gmail.com> 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 <sigaction.h>). 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" <ebiederm@xmission.com>
>> ---
>> 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);
>> }
next prev parent reply other threads:[~2018-10-05 6:27 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-25 17:16 [REVIEW][PATCH 0/6] signal: Shrinking the kernel's siginfo structure Eric W. Biederman
2018-09-25 17:16 ` Eric W. Biederman
2018-09-25 17:19 ` [REVIEW][PATCH 1/6] signal/sparc: Move EMT_TAGOVF into the generic siginfo.h Eric W. Biederman
2018-09-25 17:19 ` Eric W. Biederman
2018-09-25 17:19 ` [REVIEW][PATCH 2/6] signal: Fail sigqueueinfo if si_signo != sig Eric W. Biederman
2018-09-25 17:19 ` Eric W. Biederman
2018-10-05 6:06 ` Andrei Vagin
2018-10-05 6:06 ` Andrei Vagin
2018-10-05 6:27 ` Eric W. Biederman [this message]
2018-10-05 6:27 ` Eric W. Biederman
2018-10-05 6:50 ` Eric W. Biederman
2018-10-05 6:50 ` Eric W. Biederman
2018-10-05 7:10 ` [REVIEW][PATCH 7/6] signal: In sigqueueinfo prefer sig not si_signo Eric W. Biederman
2018-10-05 7:10 ` Eric W. Biederman
2018-09-25 17:19 ` [REVIEW][PATCH 3/6] signal: Remove the need for __ARCH_SI_PREABLE_SIZE and SI_PAD_SIZE Eric W. Biederman
2018-09-25 17:19 ` Eric W. Biederman
2018-09-25 17:19 ` [REVIEW][PATCH 4/6] signal: Introduce copy_siginfo_from_user and use it's return value Eric W. Biederman
2018-09-25 17:19 ` Eric W. Biederman
2018-09-25 17:19 ` [REVIEW][PATCH 5/6] signal: Distinguish between kernel_siginfo and siginfo Eric W. Biederman
2018-09-25 17:19 ` Eric W. Biederman
2018-09-25 17:19 ` [REVIEW][PATCH 6/6] signal: Use a smaller struct siginfo in the kernel Eric W. Biederman
2018-09-25 17:19 ` Eric W. Biederman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87h8i0di1u.fsf@xmission.com \
--to=ebiederm@xmission.com \
--cc=avagin@gmail.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).