[PATCH v12 7/8] signal: define the field siginfo.si_xflags

Peter Collingbourne pcc at google.com
Mon Nov 2 23:10:57 EST 2020


On Mon, Nov 2, 2020 at 9:38 AM Dave Martin <Dave.Martin at arm.com> 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 at google.com>
> > 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



More information about the linux-arm-kernel mailing list