From: ebiederm@xmission•com (Eric W. Biederman)
To: Peter Collingbourne <pcc@google•com>
Cc: Catalin Marinas <catalin.marinas@arm•com>,
Helge Deller <deller@gmx•de>,
Kevin Brodsky <kevin.brodsky@arm•com>,
Oleg Nesterov <oleg@redhat•com>,
linux-api@vger•kernel.org,
"James E.J. Bottomley" <James.Bottomley@hansenpartnership•com>,
Kostya Serebryany <kcc@google•com>,
Linux ARM <linux-arm-kernel@lists•infradead.org>,
Andrey Konovalov <andreyknvl@google•com>,
David Spickett <david.spickett@linaro•org>,
Vincenzo Frascino <vincenzo.frascino@arm•com>,
Will Deacon <will@kernel•org>, Dave Martin <Dave.Martin@arm•com>,
Evgenii Stepanov <eugenis@google•com>,
Richard Henderson <rth@twiddle•net>
Subject: Re: [PATCH v14 5/8] signal: clear non-uapi flag bits when passing/returning sa_flags
Date: Mon, 09 Nov 2020 18:35:30 -0600 [thread overview]
Message-ID: <871rh22gml.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <039adb228822eb1f0c90cbfc716fc28fa298c58e.1604523707.git.pcc@google.com> (Peter Collingbourne's message of "Wed, 4 Nov 2020 13:18:08 -0800")
Peter Collingbourne <pcc@google•com> writes:
2> Previously we were not clearing non-uapi flag bits in
> sigaction.sa_flags when storing the userspace-provided sa_flags or
> when returning them via oldact. Start doing so.
>
> This allows userspace to detect missing support for flag bits and
> allows the kernel to use non-uapi bits internally, as we are already
> doing in arch/x86 for two flag bits. Now that this change is in
> place, we no longer need the code in arch/x86 that was hiding these
> bits from userspace, so remove it.
>
> This is technically a userspace-visible behavior change for sigaction, as
> the unknown bits returned via oldact.sa_flags are no longer set. However,
> we are free to define the behavior for unknown bits exactly because
> their behavior is currently undefined, so for now we can define the
> meaning of each of them to be "clear the bit in oldact.sa_flags unless
> the bit becomes known in the future". Furthermore, this behavior is
> consistent with OpenBSD [1], illumos [2] and XNU [3] (FreeBSD [4] and
> NetBSD [5] fail the syscall if unknown bits are set). So there is some
> precedent for this behavior in other kernels, and in particular in XNU,
> which is probably the most popular kernel among those that I looked at,
> which means that this change is less likely to be a compatibility
> issue.
Acked-by: "Eric W. Biederman" <ebiederm@xmission•com>
Yes. The x86_64 case and general good sense suggests that this patch
will be harmless.
I suspect we should at least try and take this one step farther
(with an additional patch) that causes an error if any new flags
are passed. Or do we know of some userspace application that
currently passes bogus flags?
> Link: [1] https://github.com/openbsd/src/blob/f634a6a4b5bf832e9c1de77f7894ae2625e74484/sys/kern/kern_sig.c#L278
> Link: [2] https://github.com/illumos/illumos-gate/blob/76f19f5fdc974fe5be5c82a556e43a4df93f1de1/usr/src/uts/common/syscall/sigaction.c#L86
> Link: [3] https://github.com/apple/darwin-xnu/blob/a449c6a3b8014d9406c2ddbdc81795da24aa7443/bsd/kern/kern_sig.c#L480
> Link: [4] https://github.com/freebsd/freebsd/blob/eded70c37057857c6e23fae51f86b8f8f43cd2d0/sys/kern/kern_sig.c#L699
> Link: [5] https://github.com/NetBSD/src/blob/3365779becdcedfca206091a645a0e8e22b2946e/sys/kern/sys_sig.c#L473
> Signed-off-by: Peter Collingbourne <pcc@google•com>
> Reviewed-by: Dave Martin <Dave.Martin@arm•com>
> Link: https://linux-review.googlesource.com/id/I35aab6f5be932505d90f3b3450c083b4db1eca86
> ---
> v10:
> - rename SA_UAPI_FLAGS -> UAPI_SA_FLAGS
> - refactor how we define it to avoid mentioning flags more
> than once
>
> arch/arm/include/asm/signal.h | 2 ++
> arch/parisc/include/asm/signal.h | 2 ++
> arch/x86/kernel/signal_compat.c | 7 -------
> include/linux/signal_types.h | 12 ++++++++++++
> kernel/signal.c | 10 ++++++++++
> 5 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h
> index 65530a042009..430be7774402 100644
> --- a/arch/arm/include/asm/signal.h
> +++ b/arch/arm/include/asm/signal.h
> @@ -17,6 +17,8 @@ typedef struct {
> unsigned long sig[_NSIG_WORDS];
> } sigset_t;
>
> +#define __ARCH_UAPI_SA_FLAGS (SA_THIRTYTWO | SA_RESTORER)
> +
> #define __ARCH_HAS_SA_RESTORER
>
> #include <asm/sigcontext.h>
> diff --git a/arch/parisc/include/asm/signal.h b/arch/parisc/include/asm/signal.h
> index 715c96ba2ec8..30dd1e43ef88 100644
> --- a/arch/parisc/include/asm/signal.h
> +++ b/arch/parisc/include/asm/signal.h
> @@ -21,6 +21,8 @@ typedef struct {
> unsigned long sig[_NSIG_WORDS];
> } sigset_t;
>
> +#define __ARCH_UAPI_SA_FLAGS _SA_SIGGFAULT
> +
> #include <asm/sigcontext.h>
>
> #endif /* !__ASSEMBLY */
> diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c
> index a7f3e12cfbdb..ddfd919be46c 100644
> --- a/arch/x86/kernel/signal_compat.c
> +++ b/arch/x86/kernel/signal_compat.c
> @@ -165,16 +165,9 @@ void sigaction_compat_abi(struct k_sigaction *act, struct k_sigaction *oact)
> {
> signal_compat_build_tests();
>
> - /* Don't leak in-kernel non-uapi flags to user-space */
> - if (oact)
> - oact->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI);
> -
> if (!act)
> return;
>
> - /* Don't let flags to be set from userspace */
> - act->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI);
> -
> if (in_ia32_syscall())
> act->sa.sa_flags |= SA_IA32_ABI;
> if (in_x32_syscall())
> diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h
> index f8a90ae9c6ec..a7887ad84d36 100644
> --- a/include/linux/signal_types.h
> +++ b/include/linux/signal_types.h
> @@ -68,4 +68,16 @@ struct ksignal {
> int sig;
> };
>
> +#ifndef __ARCH_UAPI_SA_FLAGS
> +#ifdef SA_RESTORER
> +#define __ARCH_UAPI_SA_FLAGS SA_RESTORER
> +#else
> +#define __ARCH_UAPI_SA_FLAGS 0
> +#endif
> +#endif
> +
> +#define UAPI_SA_FLAGS \
> + (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \
> + SA_NODEFER | SA_RESETHAND | __ARCH_UAPI_SA_FLAGS)
> +
> #endif /* _LINUX_SIGNAL_TYPES_H */
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 74e7315c24db..832b654dee8c 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3964,6 +3964,16 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
> if (oact)
> *oact = *k;
>
> + /*
> + * Clear unknown flag bits in order to allow userspace to detect missing
> + * support for flag bits and to allow the kernel to use non-uapi bits
> + * internally.
> + */
> + if (act)
> + act->sa.sa_flags &= UAPI_SA_FLAGS;
> + if (oact)
> + oact->sa.sa_flags &= UAPI_SA_FLAGS;
> +
> sigaction_compat_abi(act, oact);
>
> if (act) {
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-11-10 0:36 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-04 21:18 [PATCH v14 0/8] arm64: expose FAR_EL1 tag bits in siginfo Peter Collingbourne
2020-11-04 21:18 ` [PATCH v14 1/8] parisc: Drop parisc special case for __sighandler_t Peter Collingbourne
2020-11-04 21:18 ` [PATCH v14 2/8] parisc: start using signal-defs.h Peter Collingbourne
2020-11-04 21:18 ` [PATCH v14 3/8] arch: move SA_* definitions to generic headers Peter Collingbourne
2020-11-04 21:18 ` [PATCH v14 4/8] signal: deduplicate code dealing with common _sigfault fields Peter Collingbourne
2020-11-10 0:41 ` Eric W. Biederman
2020-11-10 2:37 ` Peter Collingbourne
2020-11-10 15:38 ` Eric W. Biederman
2020-11-04 21:18 ` [PATCH v14 5/8] signal: clear non-uapi flag bits when passing/returning sa_flags Peter Collingbourne
2020-11-10 0:35 ` Eric W. Biederman [this message]
2020-11-10 2:19 ` Peter Collingbourne
2020-11-04 21:18 ` [PATCH v14 6/8] signal: define the SA_UNSUPPORTED bit in sa_flags Peter Collingbourne
2020-11-04 21:18 ` [PATCH v14 7/8] signal: define the field siginfo.si_faultflags Peter Collingbourne
2020-11-10 1:54 ` Eric W. Biederman
2020-11-11 11:10 ` Haren Myneni
2020-11-11 20:46 ` Eric W. Biederman
2020-11-10 1:57 ` Eric W. Biederman
2020-11-11 17:27 ` Dave Martin
2020-11-11 20:15 ` Eric W. Biederman
2020-11-11 20:28 ` Eric W. Biederman
2020-11-12 17:21 ` Dave Martin
2020-11-12 17:23 ` Dave Martin
2020-11-12 20:01 ` Eric W. Biederman
2020-11-04 21:18 ` [PATCH v14 8/8] arm64: expose FAR_EL1 tag bits in siginfo Peter Collingbourne
2020-11-10 1:13 ` Eric W. Biederman
2020-11-10 3:49 ` Peter Collingbourne
2020-11-10 15:12 ` Eric W. Biederman
2020-11-10 22:06 ` Peter Collingbourne
2020-11-11 7:45 ` Eric W. Biederman
2020-11-11 17:46 ` Dave Martin
2020-11-12 23:20 ` Peter Collingbourne
2020-11-12 18:53 ` Catalin Marinas
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=871rh22gml.fsf@x220.int.ebiederm.org \
--to=ebiederm@xmission$(echo .)com \
--cc=Dave.Martin@arm$(echo .)com \
--cc=James.Bottomley@hansenpartnership$(echo .)com \
--cc=andreyknvl@google$(echo .)com \
--cc=catalin.marinas@arm$(echo .)com \
--cc=david.spickett@linaro$(echo .)org \
--cc=deller@gmx$(echo .)de \
--cc=eugenis@google$(echo .)com \
--cc=kcc@google$(echo .)com \
--cc=kevin.brodsky@arm$(echo .)com \
--cc=linux-api@vger$(echo .)kernel.org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=oleg@redhat$(echo .)com \
--cc=pcc@google$(echo .)com \
--cc=rth@twiddle$(echo .)net \
--cc=vincenzo.frascino@arm$(echo .)com \
--cc=will@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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