On Tue, Sep 8, 2020 at 8:12 AM Dave Martin <Dave.Martin@xxxxxxx> wrote: > > On Fri, Aug 21, 2020 at 10:10:13PM -0700, Peter Collingbourne wrote: > > 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. > > > > 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@xxxxxxxxxx> > > --- > > View this change in Gerrit: https://linux-review.googlesource.com/q/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 9ccbf0576cd0..c599013ae8cb 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) > > + > > Part of me wants this to be closer to the common flag definitions. But > we don't really want to define this in the UAPI headers. Agreed on keeping this out of uapi, otherwise I would have defined this inline. > Unless you can think of another good place to put it, this is probably > OK as-is. Ack. Peter
