On Mon, Nov 2, 2020 at 9:38 AM Dave Martin <Dave.Martin@xxxxxxx> wrote:
>
> On Fri, Oct 16, 2020 at 05:12:32PM -0700, Peter Collingbourne wrote:
> > This field will contain flags that may be used by signal handlers to
> > determine whether other fields in the _sigfault portion of siginfo are
> > valid. An example use case is the following patch, which introduces
> > the si_addr_tag_bits{,_mask} fields.
> >
> > A new sigcontext flag, SA_XFLAGS, is introduced in order to allow
> > a signal handler to require the kernel to set the field (but note
> > that the field will be set anyway if the kernel supports the flag,
> > regardless of its value). In combination with the previous patches,
> > this allows a userspace program to determine whether the kernel will
> > set the field.
>
> Apologies for this coming rather late:
>
> It occurs to me that we might want a more specific name, since this only
> applies to fault signals -- say, SA_FAULTFLAGS.
>
> If we end up wanting to add flags fields for other signal types, then we
> might end up needing a SA_ flag for each, which would be a bit annoying.
>
> So, alternatively. I wonder whether it's worth preemptively adding an
> extra flags to every kind of kernel-generated siginfo. If so, then
> having a single SA_XFLAGS would be fine.
>
>
> If added flags fields all over the place is considered overkill, then I
> guess it's sufficient to rename this flag.
>
> If renaming, the actual flags field in siginfo should also be renamed to
> match.
I'd prefer not to add flags fields to every union member at this
point. I agree that faultflags is a better name, and I guess it's one
more reason not to try and reuse the ia64 field. Renamed in v13.
> >
> > It is possible for an si_xflags-unaware program to cause a signal
> > handler in an si_xflags-aware program to be called with a provided
> > siginfo data structure by using one of the following syscalls:
> >
> > - ptrace(PTRACE_SETSIGINFO)
> > - pidfd_send_signal
> > - rt_sigqueueinfo
> > - rt_tgsigqueueinfo
> >
> > So we need to prevent the si_xflags-unaware program from causing an
> > uninitialized read of si_xflags in the si_xflags-aware program when
> > it uses one of these syscalls.
> >
> > The last three cases can be handled by observing that each of these
> > syscalls fails if si_code >= 0. We also observe that kill(2) and
> > tgkill(2) may be used to send a signal where si_code == 0 (SI_USER),
> > so we define si_xflags to only be valid if si_code > 0.
> >
> > There is no such check on si_code in ptrace(PTRACE_SETSIGINFO), so
> > we make ptrace(PTRACE_SETSIGINFO) clear the si_xflags field if it
> > detects that the signal would use the _sigfault layout, and introduce
> > a new ptrace request type, PTRACE_SETSIGINFO2, that a si_xflags-aware
> > program may use to opt out of this behavior.
> >
> > It is also possible for the kernel to inject a signal specified to
> > use _sigfault by calling force_sig (e.g. there are numerous calls to
> > force_sig(SIGSEGV)). In this case si_code is set to SI_KERNEL and the
> > _kill union member is used, so document that si_code must be < SI_KERNEL.
> >
> > Ideally this field could have just been named si_flags, but that
> > name was already taken by ia64, so a different name was chosen.
> >
> > I considered making ia64's si_flags a generic field and having it
> > appear at the end of _sigfault (in the same place as this patch has
> > si_xflags) on non-ia64, keeping it in the same place on ia64. ia64's
> > si_flags is a 32-bit field with only one flag bit allocated, so we
> > would have 31 bits to use if we do this. However, it seems simplest
> > to avoid entangling these fields.
> >
> > Signed-off-by: Peter Collingbourne <pcc@xxxxxxxxxx>
> > Link: https://linux-review.googlesource.com/id/Ide155ce29366c3eab2a944ae4c51205982e5b8b2
> > ---
> > v12:
> > - Change type of si_xflags to u32 to avoid increasing alignment
> > - Add si_xflags to signal_compat.c test cases
> >
> > v11:
> > - update comment to say that si_code must > 0
> > - change ptrace(PTRACE_SETSIGINFO2) to take a flags argument
> >
> > v10:
> > - make the new field compatible with the various ways
> > that a siginfo can be injected from another process
> > - eliminate some duplication by adding a refactoring patch
> > before this one
> >
> > arch/powerpc/platforms/powernv/vas-fault.c | 1 +
> > arch/x86/kernel/signal_compat.c | 7 +++--
> > include/linux/compat.h | 2 ++
> > include/linux/signal_types.h | 2 +-
> > include/uapi/asm-generic/siginfo.h | 4 +++
> > include/uapi/asm-generic/signal-defs.h | 4 +++
> > include/uapi/linux/ptrace.h | 12 ++++++++
> > kernel/ptrace.c | 32 ++++++++++++++++++----
> > kernel/signal.c | 3 ++
> > 9 files changed, 58 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 3d21fce254b7..3bbb335561f5 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -154,6 +154,7 @@ static void update_csb(struct vas_window *window,
> > info.si_errno = EFAULT;
> > info.si_code = SEGV_MAPERR;
> > info.si_addr = csb_addr;
> > + info.si_xflags = 0;
> >
> > /*
> > * process will be polling on csb.flags after request is sent to
> > diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c
> > index ddfd919be46c..243a8cc3b41b 100644
> > --- a/arch/x86/kernel/signal_compat.c
> > +++ b/arch/x86/kernel/signal_compat.c
> > @@ -121,8 +121,8 @@ static inline void signal_compat_build_tests(void)
> > #endif
> >
> > CHECK_CSI_OFFSET(_sigfault);
> > - CHECK_CSI_SIZE (_sigfault, 4*sizeof(int));
> > - CHECK_SI_SIZE (_sigfault, 8*sizeof(int));
> > + CHECK_CSI_SIZE (_sigfault, 8*sizeof(int));
> > + CHECK_SI_SIZE (_sigfault, 16*sizeof(int));
> >
> > BUILD_BUG_ON(offsetof(siginfo_t, si_addr) != 0x10);
> > BUILD_BUG_ON(offsetof(compat_siginfo_t, si_addr) != 0x0C);
> > @@ -138,6 +138,9 @@ static inline void signal_compat_build_tests(void)
> > BUILD_BUG_ON(offsetof(siginfo_t, si_pkey) != 0x20);
> > BUILD_BUG_ON(offsetof(compat_siginfo_t, si_pkey) != 0x14);
> >
> > + BUILD_BUG_ON(offsetof(siginfo_t, si_xflags) != 0x48);
> > + BUILD_BUG_ON(offsetof(compat_siginfo_t, si_xflags) != 0x28);
> > +
> > CHECK_CSI_OFFSET(_sigpoll);
> > CHECK_CSI_SIZE (_sigpoll, 2*sizeof(int));
> > CHECK_SI_SIZE (_sigpoll, 4*sizeof(int));
> > diff --git a/include/linux/compat.h b/include/linux/compat.h
> > index 14d514233e1d..ea77a24ce6a2 100644
> > --- a/include/linux/compat.h
> > +++ b/include/linux/compat.h
> > @@ -236,7 +236,9 @@ typedef struct compat_siginfo {
> > char _dummy_pkey[__COMPAT_ADDR_BND_PKEY_PAD];
> > u32 _pkey;
> > } _addr_pkey;
> > + compat_uptr_t _pad[6];
> > };
> > + u32 _xflags;
> > } _sigfault;
> >
> > /* SIGPOLL */
> > diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h
> > index a7887ad84d36..75ca861d982a 100644
> > --- a/include/linux/signal_types.h
> > +++ b/include/linux/signal_types.h
> > @@ -78,6 +78,6 @@ struct ksignal {
> >
> > #define UAPI_SA_FLAGS \
> > (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \
> > - SA_NODEFER | SA_RESETHAND | __ARCH_UAPI_SA_FLAGS)
> > + SA_NODEFER | SA_RESETHAND | SA_XFLAGS | __ARCH_UAPI_SA_FLAGS)
> >
> > #endif /* _LINUX_SIGNAL_TYPES_H */
> > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> > index 7aacf9389010..8158d5df666a 100644
> > --- a/include/uapi/asm-generic/siginfo.h
> > +++ b/include/uapi/asm-generic/siginfo.h
> > @@ -91,7 +91,9 @@ union __sifields {
> > char _dummy_pkey[__ADDR_BND_PKEY_PAD];
> > __u32 _pkey;
> > } _addr_pkey;
> > + void *_pad[6];
> > };
> > + __u32 _xflags;
> > } _sigfault;
> >
> > /* SIGPOLL */
> > @@ -152,6 +154,8 @@ typedef struct siginfo {
> > #define si_trapno _sifields._sigfault._trapno
> > #endif
> > #define si_addr_lsb _sifields._sigfault._addr_lsb
> > +/* si_xflags is only valid if 0 < si_code < SI_KERNEL */
> > +#define si_xflags _sifields._sigfault._xflags
> > #define si_lower _sifields._sigfault._addr_bnd._lower
> > #define si_upper _sifields._sigfault._addr_bnd._upper
> > #define si_pkey _sifields._sigfault._addr_pkey._pkey
> > diff --git a/include/uapi/asm-generic/signal-defs.h b/include/uapi/asm-generic/signal-defs.h
> > index 0126ebda4d31..cd522819f4ba 100644
> > --- a/include/uapi/asm-generic/signal-defs.h
> > +++ b/include/uapi/asm-generic/signal-defs.h
> > @@ -20,6 +20,9 @@
> > * so this bit allows flag bit support to be detected from userspace while
> > * allowing an old kernel to be distinguished from a kernel that supports every
> > * flag bit.
> > + * SA_XFLAGS indicates that the signal handler requires the siginfo.si_xflags
> > + * field to be valid. Note that if the kernel supports SA_XFLAGS, the field will
> > + * be valid regardless of the value of this flag.
> > *
> > * SA_ONESHOT and SA_NOMASK are the historical Linux names for the Single
> > * Unix names RESETHAND and NODEFER respectively.
> > @@ -49,6 +52,7 @@
> > #define SA_RESETHAND 0x80000000
> > #endif
> > #define SA_UNSUPPORTED 0x00000400
> > +#define SA_XFLAGS 0x00000800
> >
> > #define SA_NOMASK SA_NODEFER
> > #define SA_ONESHOT SA_RESETHAND
> > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > index a71b6e3b03eb..93946edf0139 100644
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -101,6 +101,18 @@ struct ptrace_syscall_info {
> > };
> > };
> >
> > +#define PTRACE_SETSIGINFO2 0x420f
> > +/*
> > + * These flags are passed as the addr argument to ptrace.
> > + */
> > +
> > +/*
> > + * Asserts that the caller is aware of the field siginfo.si_xflags. Prevents
> > + * the kernel from automatically setting the field to 0 when the signal uses
> > + * a sigfault layout.
> > + */
> > +#define PTRACE_SIGINFO_XFLAGS 0x1
> > +
> > /*
> > * These values are stored in task->ptrace_message
> > * by tracehook_report_syscall_* to describe the current syscall-stop.
> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index 43d6179508d6..85b5b4e38661 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -687,18 +687,32 @@ static int ptrace_getsiginfo(struct task_struct *child, kernel_siginfo_t *info)
> > return error;
> > }
> >
> > -static int ptrace_setsiginfo(struct task_struct *child, const kernel_siginfo_t *info)
> > +static int ptrace_setsiginfo(struct task_struct *child, unsigned long flags,
> > + kernel_siginfo_t *info)
> > {
> > - unsigned long flags;
> > + unsigned long lock_flags;
> > int error = -ESRCH;
> >
> > - if (lock_task_sighand(child, &flags)) {
> > + if (flags & ~PTRACE_SIGINFO_XFLAGS) {
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * If the caller is unaware of si_xflags and we're using a layout that
> > + * requires it, set it to 0 which means "no fields are available".
> > + */
> > + if (!(flags & PTRACE_SIGINFO_XFLAGS) &&
> > + siginfo_layout_is_fault(
> > + siginfo_layout(info->si_signo, info->si_code)))
> > + info->si_xflags = 0;
> > +
> > + if (lock_task_sighand(child, &lock_flags)) {
> > error = -EINVAL;
> > if (likely(child->last_siginfo != NULL)) {
> > copy_siginfo(child->last_siginfo, info);
> > error = 0;
> > }
> > - unlock_task_sighand(child, &flags);
> > + unlock_task_sighand(child, &lock_flags);
> > }
> > return error;
> > }
> > @@ -1038,9 +1052,12 @@ int ptrace_request(struct task_struct *child, long request,
> > break;
> >
> > case PTRACE_SETSIGINFO:
> > + addr = 0;
>
> If this is intended to fall through, please add a
>
> /* fall through */
>
> comment here (newer GCC has warnings to catch this; not sure about
> clang, but I'd be surprised if no version warns).
Yes, clang has this warning, but it looks like it is currently
disabled in clang due to differences between the compilers [1] so I
didn't see it.
It looks like the kernel is moving towards using the fallthrough
macro/attribute defined in include/linux/compiler_attributes.h (and to
me this personally seems better than relying on parsing comments), so
I've used that macro in v13.
Peter
[1] https://github.com/ClangBuiltLinux/linux/issues/636
