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.
>
> 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).
[...]
Otherwise, the patch looks reasonable, but I'll wait for your response
on the above points before reviewing in detail.
Cheers
---Dave
