This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4 2/4] Filter out NPTL internal signals (BZ #22391)
Ping.
On 12/02/2018 10:42, Adhemerval Zanella wrote:
> This patch filters out the internal NPTL signals (SIGCANCEL/SIGTIMER and
> SIGSETXID) from signal functions. GLIBC on Linux requires both signals to
> proper implement pthread cancellation, posix timers, and set*id posix
> thread synchronization.
>
> And not filtering out the internal signal is troublesome:
>
> - A conformant program on a architecture that does not filter out the
> signals might inadvertently disable pthread asynchronous cancellation,
> set*id synchronization or posix timers.
>
> - It might also to security issues if SIGSETXID is masked and set*id
> functions are called (some threads might have effective user or group
> id different from the rest).
>
> The changes are basically:
>
> - Change __is_internal_signal to bool and used on all signal function
> that has a signal number as input. Also for signal function which accepts
> signals sets (sigset_t) it assumes that canonical function were used to
> add/remove signals which lead to some input simplification.
>
> - Fix tst-sigset.c to avoid check for SIGCANCEL/SIGTIMER and SIGSETXID.
> It is rewritten to check each signal indidually and to check realtime
> signals using canonical macros.
>
> - Add generic __clear_internal_signals and __is_internal_signal
> version since both symbols are used on generic implementations.
>
> - Remove superflous sysdeps/nptl/sigfillset.c.
>
> - Remove superflous SIGTIMER handling on Linux __is_internal_signal
> since it is the same of SIGCANCEL.
>
> - Remove dangling define and obvious comment on nptl/sigaction.c.
>
> Checked on x86_64-linux-gnu.
>
> [BZ #22391]
> * nptl/sigaction.c (__sigaction): Use __is_internal_signal to
> check for internal nptl signals.
> * signal/sigaddset.c (sigaddset): Likewise.
> * signal/sigdelset.c (sigdelset): Likewise.
> * sysdeps/posix/signal.c (__bsd_signal): Likewise.
> * sysdeps/posix/sigset.c (sigset): Call and check sigaddset return
> value.
> * signal/sigfillset.c (sigfillset): User __clear_internal_signals
> to filter out internal nptl signals.
> * signal/tst-sigset.c (do_test): Check ech signal indidually and
> also check realtime signals using standard macros.
> * sysdeps/nptl/nptl-signals.h (__clear_internal_signals,
> __is_internal_signal): New functions.
> * sysdeps/nptl/sigfillset.c: Remove file.
> * sysdeps/unix/sysv/linux/nptl-signals.h (__is_internal_signal):
> Change return to bool.
> (__clear_internal_signals): Remove SIGTIMER clean since it is
> equal to SIGCANEL on Linux.
> * sysdeps/unix/sysv/linux/sigtimedwait.c (__sigtimedwait): Assume
> signal set was constructed using standard functions.
> * sysdeps/unix/sysv/linux/sigwait.c (do_sigtwait): Likewise.
>
> Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Reported-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
> ChangeLog | 23 ++++++++
> nptl/sigaction.c | 14 +----
> signal/sigaction.c | 2 +-
> signal/sigaddset.c | 5 +-
> signal/sigdelset.c | 5 +-
> signal/sigfillset.c | 10 +---
> signal/tst-sigset.c | 92 ++++++++++++++++++++++--------
> sysdeps/generic/internal-signals.h | 11 ++++
> sysdeps/nptl/sigfillset.c | 20 -------
> sysdeps/posix/signal.c | 5 +-
> sysdeps/posix/sigset.c | 10 +---
> sysdeps/unix/sysv/linux/internal-signals.h | 4 +-
> sysdeps/unix/sysv/linux/sigtimedwait.c | 17 +-----
> 13 files changed, 122 insertions(+), 96 deletions(-)
> delete mode 100644 sysdeps/nptl/sigfillset.c
>
> diff --git a/nptl/sigaction.c b/nptl/sigaction.c
> index ddf6f5e..79b6fdc 100644
> --- a/nptl/sigaction.c
> +++ b/nptl/sigaction.c
> @@ -16,22 +16,12 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> -
> -/* This is no complete implementation. The file is meant to be
> - included in the real implementation to provide the wrapper around
> - __libc_sigaction. */
> -
> -#include <nptl/pthreadP.h>
> -
> -/* We use the libc implementation but we tell it to not allow
> - SIGCANCEL or SIGTIMER to be handled. */
> -#define LIBC_SIGACTION 1
> -
> +#include <internal-signals.h>
>
> int
> __sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
> {
> - if (__glibc_unlikely (sig == SIGCANCEL || sig == SIGSETXID))
> + if (sig <= 0 || sig >= NSIG || __is_internal_signal (sig))
> {
> __set_errno (EINVAL);
> return -1;
> diff --git a/signal/sigaction.c b/signal/sigaction.c
> index f761ca2..c99001a 100644
> --- a/signal/sigaction.c
> +++ b/signal/sigaction.c
> @@ -24,7 +24,7 @@
> int
> __sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
> {
> - if (sig <= 0 || sig >= NSIG)
> + if (sig <= 0 || sig >= NSIG || __is_internal_signal (sig))
> {
> __set_errno (EINVAL);
> return -1;
> diff --git a/signal/sigaddset.c b/signal/sigaddset.c
> index d310890..7238df4 100644
> --- a/signal/sigaddset.c
> +++ b/signal/sigaddset.c
> @@ -17,13 +17,14 @@
>
> #include <errno.h>
> #include <signal.h>
> -#include <sigsetops.h>
> +#include <internal-signals.h>
>
> /* Add SIGNO to SET. */
> int
> sigaddset (sigset_t *set, int signo)
> {
> - if (set == NULL || signo <= 0 || signo >= NSIG)
> + if (set == NULL || signo <= 0 || signo >= NSIG
> + || __is_internal_signal (signo))
> {
> __set_errno (EINVAL);
> return -1;
> diff --git a/signal/sigdelset.c b/signal/sigdelset.c
> index cd83dda..011978c 100644
> --- a/signal/sigdelset.c
> +++ b/signal/sigdelset.c
> @@ -17,13 +17,14 @@
>
> #include <errno.h>
> #include <signal.h>
> -#include <sigsetops.h>
> +#include <internal-signals.h>
>
> /* Add SIGNO to SET. */
> int
> sigdelset (sigset_t *set, int signo)
> {
> - if (set == NULL || signo <= 0 || signo >= NSIG)
> + if (set == NULL || signo <= 0 || signo >= NSIG
> + || __is_internal_signal (signo))
> {
> __set_errno (EINVAL);
> return -1;
> diff --git a/signal/sigfillset.c b/signal/sigfillset.c
> index e586fd9..83dd583 100644
> --- a/signal/sigfillset.c
> +++ b/signal/sigfillset.c
> @@ -18,6 +18,7 @@
> #include <errno.h>
> #include <signal.h>
> #include <string.h>
> +#include <internal-signals.h>
>
> /* Set all signals in SET. */
> int
> @@ -31,14 +32,7 @@ sigfillset (sigset_t *set)
>
> memset (set, 0xff, sizeof (sigset_t));
>
> - /* If the implementation uses a cancellation signal don't set the bit. */
> -#ifdef SIGCANCEL
> - __sigdelset (set, SIGCANCEL);
> -#endif
> - /* Likewise for the signal to implement setxid. */
> -#ifdef SIGSETXID
> - __sigdelset (set, SIGSETXID);
> -#endif
> + __clear_internal_signals (set);
>
> return 0;
> }
> diff --git a/signal/tst-sigset.c b/signal/tst-sigset.c
> index d47adcc..a2b764d 100644
> --- a/signal/tst-sigset.c
> +++ b/signal/tst-sigset.c
> @@ -1,43 +1,85 @@
> /* Test sig*set functions. */
>
> #include <signal.h>
> -#include <stdio.h>
>
> -#define TEST_FUNCTION do_test ()
> +#include <support/check.h>
> +
> static int
> do_test (void)
> {
> - int result = 0;
> - int sig = -1;
> + sigset_t set;
> + TEST_VERIFY (sigemptyset (&set) == 0);
>
> -#define TRY(call) \
> - if (call) \
> - { \
> - printf ("%s (sig = %d): %m\n", #call, sig); \
> - result = 1; \
> - } \
> - else
> +#define VERIFY(set, sig) \
> + TEST_VERIFY (sigismember (&set, sig) == 0); \
> + TEST_VERIFY (sigaddset (&set, sig) == 0); \
> + TEST_VERIFY (sigismember (&set, sig) != 0); \
> + TEST_VERIFY (sigdelset (&set, sig) == 0); \
> + TEST_VERIFY (sigismember (&set, sig) == 0)
>
> + /* ISO C99 signals. */
> + VERIFY (set, SIGINT);
> + VERIFY (set, SIGILL);
> + VERIFY (set, SIGABRT);
> + VERIFY (set, SIGFPE);
> + VERIFY (set, SIGSEGV);
> + VERIFY (set, SIGTERM);
>
> - sigset_t set;
> - TRY (sigemptyset (&set) != 0);
> + /* Historical signals specified by POSIX. */
> + VERIFY (set, SIGHUP);
> + VERIFY (set, SIGQUIT);
> + VERIFY (set, SIGTRAP);
> + VERIFY (set, SIGKILL);
> + VERIFY (set, SIGBUS);
> + VERIFY (set, SIGSYS);
> + VERIFY (set, SIGPIPE);
> + VERIFY (set, SIGALRM);
> +
> + /* New(er) POSIX signals (1003.1-2008, 1003.1-2013). */
> + VERIFY (set, SIGURG);
> + VERIFY (set, SIGSTOP);
> + VERIFY (set, SIGTSTP);
> + VERIFY (set, SIGCONT);
> + VERIFY (set, SIGCHLD);
> + VERIFY (set, SIGTTIN);
> + VERIFY (set, SIGTTOU);
> + VERIFY (set, SIGPOLL);
> + VERIFY (set, SIGXCPU);
> + VERIFY (set, SIGXFSZ);
> + VERIFY (set, SIGVTALRM);
> + VERIFY (set, SIGPROF);
> + VERIFY (set, SIGUSR1);
> + VERIFY (set, SIGUSR2);
> +
> + /* Nonstandard signals found in all modern POSIX systems
> + (including both BSD and Linux). */
> + VERIFY (set, SIGWINCH);
>
> -#ifdef SIGRTMAX
> - int max_sig = SIGRTMAX;
> -#else
> - int max_sig = NSIG - 1;
> + /* Arch-specific signals. */
> +#ifdef SIGEMT
> + VERIFY (set, SIGEMT);
> +#endif
> +#ifdef SIGLOST
> + VERIFY (set, SIGLOST);
> +#endif
> +#ifdef SIGINFO
> + VERIFY (set, SIGINFO);
> +#endif
> +#ifdef SIGSTKFLT
> + VERIFY (set, SIGSTKFLT);
> +#endif
> +#ifdef SIGPWR
> + VERIFY (set, SIGPWR);
> #endif
>
> - for (sig = 1; sig <= max_sig; ++sig)
> + /* Read-time signals (POSIX.1b real-time extensions). If they are
> + supported SIGRTMAX value is greater than SIGRTMIN. */
> + for (int rtsig = SIGRTMIN; rtsig <= SIGRTMAX; rtsig++)
> {
> - TRY (sigismember (&set, sig) != 0);
> - TRY (sigaddset (&set, sig) != 0);
> - TRY (sigismember (&set, sig) == 0);
> - TRY (sigdelset (&set, sig) != 0);
> - TRY (sigismember (&set, sig) != 0);
> + VERIFY (set, rtsig);
> }
>
> - return result;
> + return 0;
> }
>
> -#include "../test-skeleton.c"
> +#include <support/test-driver.c>
> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
> index 01e5b75..ab0b22e 100644
> --- a/sysdeps/generic/internal-signals.h
> +++ b/sysdeps/generic/internal-signals.h
> @@ -15,3 +15,14 @@
> You should have received a copy of the GNU Lesser General Public
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
> +
> +static inline void
> +__clear_internal_signals (sigset_t *set)
> +{
> +}
> +
> +static inline bool
> +__is_internal_signal (int sig)
> +{
> + return false;
> +}
> diff --git a/sysdeps/nptl/sigfillset.c b/sysdeps/nptl/sigfillset.c
> deleted file mode 100644
> index 94a7680..0000000
> --- a/sysdeps/nptl/sigfillset.c
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -/* Copyright (C) 2003-2018 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <http://www.gnu.org/licenses/>. */
> -
> -#include <nptl/pthreadP.h>
> -
> -#include <signal/sigfillset.c>
> diff --git a/sysdeps/posix/signal.c b/sysdeps/posix/signal.c
> index a4a0875..8a135c7 100644
> --- a/sysdeps/posix/signal.c
> +++ b/sysdeps/posix/signal.c
> @@ -18,8 +18,8 @@
>
> #include <errno.h>
> #include <signal.h>
> -#include <string.h> /* For the real memset prototype. */
> #include <sigsetops.h>
> +#include <internal-signals.h>
>
> sigset_t _sigintr attribute_hidden; /* Set by siginterrupt. */
>
> @@ -31,7 +31,8 @@ __bsd_signal (int sig, __sighandler_t handler)
> struct sigaction act, oact;
>
> /* Check signal extents to protect __sigismember. */
> - if (handler == SIG_ERR || sig < 1 || sig >= NSIG)
> + if (handler == SIG_ERR || sig < 1 || sig >= NSIG
> + || __is_internal_signal (sig))
> {
> __set_errno (EINVAL);
> return SIG_ERR;
> diff --git a/sysdeps/posix/sigset.c b/sysdeps/posix/sigset.c
> index b62aa3c..6ab4a48 100644
> --- a/sysdeps/posix/sigset.c
> +++ b/sysdeps/posix/sigset.c
> @@ -31,15 +31,9 @@ sigset (int sig, __sighandler_t disp)
> sigset_t set;
> sigset_t oset;
>
> - /* Check signal extents to protect __sigismember. */
> - if (disp == SIG_ERR || sig < 1 || sig >= NSIG)
> - {
> - __set_errno (EINVAL);
> - return SIG_ERR;
> - }
> -
> __sigemptyset (&set);
> - __sigaddset (&set, sig);
> + if (sigaddset (&set, sig) < 0)
> + return SIG_ERR;
>
> if (disp == SIG_HOLD)
> {
> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
> index e007372..5ff4cf8 100644
> --- a/sysdeps/unix/sysv/linux/internal-signals.h
> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
> @@ -21,6 +21,8 @@
>
> #include <signal.h>
> #include <sigsetops.h>
> +#include <stdbool.h>
> +#include <sysdep.h>
>
> /* The signal used for asynchronous cancelation. */
> #define SIGCANCEL __SIGRTMIN
> @@ -37,7 +39,7 @@
>
>
> /* Return is sig is used internally. */
> -static inline int
> +static inline bool
> __is_internal_signal (int sig)
> {
> return (sig == SIGCANCEL) || (sig == SIGSETXID);
> diff --git a/sysdeps/unix/sysv/linux/sigtimedwait.c b/sysdeps/unix/sysv/linux/sigtimedwait.c
> index 051a285..b4de885 100644
> --- a/sysdeps/unix/sysv/linux/sigtimedwait.c
> +++ b/sysdeps/unix/sysv/linux/sigtimedwait.c
> @@ -24,21 +24,8 @@ int
> __sigtimedwait (const sigset_t *set, siginfo_t *info,
> const struct timespec *timeout)
> {
> - sigset_t tmpset;
> - if (set != NULL
> - && (__builtin_expect (__sigismember (set, SIGCANCEL), 0)
> - || __builtin_expect (__sigismember (set, SIGSETXID), 0)))
> - {
> - /* Create a temporary mask without the bit for SIGCANCEL set. */
> - // We are not copying more than we have to.
> - memcpy (&tmpset, set, _NSIG / 8);
> - __sigdelset (&tmpset, SIGCANCEL);
> - __sigdelset (&tmpset, SIGSETXID);
> - set = &tmpset;
> - }
> -
> - /* XXX The size argument hopefully will have to be changed to the
> - real size of the user-level sigset_t. */
> + /* XXX The size argument hopefully will have to be changed to the
> + real size of the user-level sigset_t. */
> int result = SYSCALL_CANCEL (rt_sigtimedwait, set, info, timeout, _NSIG / 8);
>
> /* The kernel generates a SI_TKILL code in si_code in case tkill is
>