| Message ID | e26271d2b3767cdbd265033e6f7eb28f828f3a28.1597720138.git.pcc@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: expose FAR_EL1 tag bits in siginfo | expand |
On Mon, Aug 17, 2020 at 08:33:50PM -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_ignored_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. > > Ideally this field could have just been named si_flags, but that > name was already taken by ia64, so a different name was chosen. > > Alternatively, we may consider 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. For clarity, is the new si_xflags field supposed to be valid for all signal types, or just certain signals and si_codes? What happens for things like a rt_sigqueueinfo() from userspace? > > Signed-off-by: Peter Collingbourne <pcc@google.com> > --- > View this change in Gerrit: https://linux-review.googlesource.com/q/Ide155ce29366c3eab2a944ae4c51205982e5b8b2 > > arch/arm/include/asm/signal.h | 3 ++- > arch/parisc/include/asm/signal.h | 2 +- > arch/powerpc/platforms/powernv/vas-fault.c | 1 + > include/linux/compat.h | 2 ++ > include/linux/signal_types.h | 4 ++-- > include/uapi/asm-generic/siginfo.h | 3 +++ > include/uapi/asm-generic/signal-defs.h | 4 ++++ > kernel/signal.c | 15 +++++++++++++++ > 8 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h > index d1070a783993..6b2630dfe1df 100644 > --- a/arch/arm/include/asm/signal.h > +++ b/arch/arm/include/asm/signal.h > @@ -19,7 +19,8 @@ typedef struct { > > #define SA_UAPI_FLAGS \ > (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_THIRTYTWO | \ > - SA_RESTORER | SA_ONSTACK | SA_RESTART | SA_NODEFER | SA_RESETHAND) > + SA_RESTORER | SA_ONSTACK | SA_RESTART | SA_NODEFER | SA_RESETHAND | \ > + SA_XFLAGS) > > #define __ARCH_HAS_SA_RESTORER > > diff --git a/arch/parisc/include/asm/signal.h b/arch/parisc/include/asm/signal.h > index ad06e14f6e8a..3582bce44520 100644 > --- a/arch/parisc/include/asm/signal.h > +++ b/arch/parisc/include/asm/signal.h > @@ -23,7 +23,7 @@ typedef struct { > > #define SA_UAPI_FLAGS \ > (SA_ONSTACK | SA_RESETHAND | SA_NOCLDSTOP | SA_SIGINFO | SA_NODEFER | \ > - SA_RESTART | SA_NOCLDWAIT | _SA_SIGGFAULT) > + SA_RESTART | SA_NOCLDWAIT | _SA_SIGGFAULT | SA_XFLAGS) > > #include <asm/sigcontext.h> > > 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/include/linux/compat.h b/include/linux/compat.h > index d38c4d7e83bd..55d4228dfd88 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -231,7 +231,9 @@ typedef struct compat_siginfo { > char _dummy_pkey[__COMPAT_ADDR_BND_PKEY_PAD]; > u32 _pkey; > } _addr_pkey; > + compat_uptr_t _pad[6]; > }; > + compat_uptr_t _xflags; > } _sigfault; > > /* SIGPOLL */ > diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h > index e792f29b5846..cd3d08dde47a 100644 > --- a/include/linux/signal_types.h > +++ b/include/linux/signal_types.h > @@ -72,11 +72,11 @@ struct ksignal { > #ifdef SA_RESTORER > #define SA_UAPI_FLAGS \ > (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > - SA_NODEFER | SA_RESETHAND | SA_RESTORER) > + SA_NODEFER | SA_RESETHAND | SA_RESTORER | SA_XFLAGS) > #else > #define SA_UAPI_FLAGS \ > (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > - SA_NODEFER | SA_RESETHAND) > + SA_NODEFER | SA_RESETHAND | SA_XFLAGS) > #endif > #endif > > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > index cb3d6c267181..413d804623c0 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]; > }; > + unsigned long _xflags; > } _sigfault; > > /* SIGPOLL */ > @@ -152,6 +154,7 @@ typedef struct siginfo { > #define si_trapno _sifields._sigfault._trapno > #endif > #define si_addr_lsb _sifields._sigfault._addr_lsb > +#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 c30a9c1a77b2..aeee6bb0763b 100644 > --- a/include/uapi/asm-generic/signal-defs.h > +++ b/include/uapi/asm-generic/signal-defs.h > @@ -19,6 +19,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 @@ > * should be avoided for new generic flags: 3, 4, 5, 6, 7, 8, 9, 16, 24, 25, 26. > */ > #define SA_UNSUPPORTED 0x00000400 > +#define SA_XFLAGS 0x00000800 > > #define SA_NOMASK SA_NODEFER > #define SA_ONESHOT SA_RESETHAND > diff --git a/kernel/signal.c b/kernel/signal.c > index 664a6c31137e..72182eed1b8d 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1669,6 +1669,7 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr > info.si_flags = flags; > info.si_isr = isr; > #endif > + info.si_xflags = 0; > return force_sig_info_to_task(&info, t); > } > > @@ -1701,6 +1702,7 @@ int send_sig_fault(int sig, int code, void __user *addr > info.si_flags = flags; > info.si_isr = isr; > #endif > + info.si_xflags = 0; > return send_sig_info(info.si_signo, &info, t); > } > > @@ -1715,6 +1717,7 @@ int force_sig_mceerr(int code, void __user *addr, short lsb) > info.si_code = code; > info.si_addr = addr; > info.si_addr_lsb = lsb; > + info.si_xflags = 0; > return force_sig_info(&info); > } > > @@ -1729,6 +1732,7 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct * > info.si_code = code; > info.si_addr = addr; > info.si_addr_lsb = lsb; > + info.si_xflags = 0; > return send_sig_info(info.si_signo, &info, t); > } > EXPORT_SYMBOL(send_sig_mceerr); > @@ -1744,6 +1748,7 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper) > info.si_addr = addr; > info.si_lower = lower; > info.si_upper = upper; > + info.si_xflags = 0; > return force_sig_info(&info); > } > > @@ -1758,6 +1763,7 @@ int force_sig_pkuerr(void __user *addr, u32 pkey) > info.si_code = SEGV_PKUERR; > info.si_addr = addr; > info.si_pkey = pkey; > + info.si_xflags = 0; > return force_sig_info(&info); > } > #endif > @@ -1774,6 +1780,7 @@ int force_sig_ptrace_errno_trap(int errno, void __user *addr) > info.si_errno = errno; > info.si_code = TRAP_HWBKPT; > info.si_addr = addr; > + info.si_xflags = 0; > return force_sig_info(&info); > } > > @@ -3290,6 +3297,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > #ifdef __ARCH_SI_TRAPNO > to->si_trapno = from->si_trapno; > #endif > + to->si_xflags = from->si_xflags; > break; > case SIL_FAULT_MCEERR: > to->si_addr = ptr_to_compat(from->si_addr); > @@ -3297,6 +3305,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > to->si_trapno = from->si_trapno; > #endif > to->si_addr_lsb = from->si_addr_lsb; > + to->si_xflags = from->si_xflags; > break; > case SIL_FAULT_BNDERR: > to->si_addr = ptr_to_compat(from->si_addr); > @@ -3305,6 +3314,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > #endif > to->si_lower = ptr_to_compat(from->si_lower); > to->si_upper = ptr_to_compat(from->si_upper); > + to->si_xflags = from->si_xflags; > break; > case SIL_FAULT_PKUERR: > to->si_addr = ptr_to_compat(from->si_addr); > @@ -3312,6 +3322,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > to->si_trapno = from->si_trapno; > #endif > to->si_pkey = from->si_pkey; > + to->si_xflags = from->si_xflags; > break; > case SIL_CHLD: > to->si_pid = from->si_pid; > @@ -3370,6 +3381,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > #ifdef __ARCH_SI_TRAPNO > to->si_trapno = from->si_trapno; > #endif > + to->si_xflags = from->si_xflags; > break; > case SIL_FAULT_MCEERR: > to->si_addr = compat_ptr(from->si_addr); > @@ -3377,6 +3389,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > to->si_trapno = from->si_trapno; > #endif > to->si_addr_lsb = from->si_addr_lsb; > + to->si_xflags = from->si_xflags; > break; > case SIL_FAULT_BNDERR: > to->si_addr = compat_ptr(from->si_addr); > @@ -3385,6 +3398,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > #endif > to->si_lower = compat_ptr(from->si_lower); > to->si_upper = compat_ptr(from->si_upper); > + to->si_xflags = from->si_xflags; > break; > case SIL_FAULT_PKUERR: > to->si_addr = compat_ptr(from->si_addr); > @@ -3392,6 +3406,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > to->si_trapno = from->si_trapno; > #endif > to->si_pkey = from->si_pkey; > + to->si_xflags = from->si_xflags; How did you figure out the list of places to make these changes? I'm not sure how to confirm that it's exhaustive. It's a shame if we can't simply apply the change in one place. Would the refactoring be too invasive to accomplish that? Cheers ---Dave
On Wed, Aug 19, 2020 at 8:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > On Mon, Aug 17, 2020 at 08:33:50PM -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_ignored_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. > > > > Ideally this field could have just been named si_flags, but that > > name was already taken by ia64, so a different name was chosen. > > > > Alternatively, we may consider 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. > > For clarity, is the new si_xflags field supposed to be valid for all > signal types, or just certain signals and si_codes? It is intended to be valid for all signal types that use the _sigfault union member of siginfo. As listed in siginfo.h these are: SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT. > What happens for things like a rt_sigqueueinfo() from userspace? Hmm. Let's enumerate each of these things, which I believe are all of the call sites of the function copy_siginfo_from_user and related functions (correct me if I'm wrong): - ptrace(PTRACE_SETSIGINFO) - pidfd_send_signal - rt_sigqueueinfo - rt_tgsigqueueinfo We can handle the last three by observing that the kernel forbids sending a signal with these syscalls if si_code >= 0, so we can say that the value of si_xflags is only valid if si_code >= 0. As for the first one, it's more tricky. Arguably something like a debugger should be able to send arbitrary signals to a debuggee, and there's no reason why it shouldn't be able to set si_xflags in siginfo, but on the other hand who knows how existing debuggers end up setting this field today. Maybe all that we can do is have the kernel clear si_xflags if it detects that the signal uses _sigfault, and let si_xflags aware debuggers opt out of this behavior, perhaps by introducing a PTRACE_SETSIGINFO2 or something. > > > > Signed-off-by: Peter Collingbourne <pcc@google.com> > > --- > > View this change in Gerrit: https://linux-review.googlesource.com/q/Ide155ce29366c3eab2a944ae4c51205982e5b8b2 > > > > arch/arm/include/asm/signal.h | 3 ++- > > arch/parisc/include/asm/signal.h | 2 +- > > arch/powerpc/platforms/powernv/vas-fault.c | 1 + > > include/linux/compat.h | 2 ++ > > include/linux/signal_types.h | 4 ++-- > > include/uapi/asm-generic/siginfo.h | 3 +++ > > include/uapi/asm-generic/signal-defs.h | 4 ++++ > > kernel/signal.c | 15 +++++++++++++++ > > 8 files changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h > > index d1070a783993..6b2630dfe1df 100644 > > --- a/arch/arm/include/asm/signal.h > > +++ b/arch/arm/include/asm/signal.h > > @@ -19,7 +19,8 @@ typedef struct { > > > > #define SA_UAPI_FLAGS \ > > (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_THIRTYTWO | \ > > - SA_RESTORER | SA_ONSTACK | SA_RESTART | SA_NODEFER | SA_RESETHAND) > > + SA_RESTORER | SA_ONSTACK | SA_RESTART | SA_NODEFER | SA_RESETHAND | \ > > + SA_XFLAGS) > > > > #define __ARCH_HAS_SA_RESTORER > > > > diff --git a/arch/parisc/include/asm/signal.h b/arch/parisc/include/asm/signal.h > > index ad06e14f6e8a..3582bce44520 100644 > > --- a/arch/parisc/include/asm/signal.h > > +++ b/arch/parisc/include/asm/signal.h > > @@ -23,7 +23,7 @@ typedef struct { > > > > #define SA_UAPI_FLAGS \ > > (SA_ONSTACK | SA_RESETHAND | SA_NOCLDSTOP | SA_SIGINFO | SA_NODEFER | \ > > - SA_RESTART | SA_NOCLDWAIT | _SA_SIGGFAULT) > > + SA_RESTART | SA_NOCLDWAIT | _SA_SIGGFAULT | SA_XFLAGS) > > > > #include <asm/sigcontext.h> > > > > 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/include/linux/compat.h b/include/linux/compat.h > > index d38c4d7e83bd..55d4228dfd88 100644 > > --- a/include/linux/compat.h > > +++ b/include/linux/compat.h > > @@ -231,7 +231,9 @@ typedef struct compat_siginfo { > > char _dummy_pkey[__COMPAT_ADDR_BND_PKEY_PAD]; > > u32 _pkey; > > } _addr_pkey; > > + compat_uptr_t _pad[6]; > > }; > > + compat_uptr_t _xflags; > > } _sigfault; > > > > /* SIGPOLL */ > > diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h > > index e792f29b5846..cd3d08dde47a 100644 > > --- a/include/linux/signal_types.h > > +++ b/include/linux/signal_types.h > > @@ -72,11 +72,11 @@ struct ksignal { > > #ifdef SA_RESTORER > > #define SA_UAPI_FLAGS \ > > (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > > - SA_NODEFER | SA_RESETHAND | SA_RESTORER) > > + SA_NODEFER | SA_RESETHAND | SA_RESTORER | SA_XFLAGS) > > #else > > #define SA_UAPI_FLAGS \ > > (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > > - SA_NODEFER | SA_RESETHAND) > > + SA_NODEFER | SA_RESETHAND | SA_XFLAGS) > > #endif > > #endif > > > > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > > index cb3d6c267181..413d804623c0 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]; > > }; > > + unsigned long _xflags; > > } _sigfault; > > > > /* SIGPOLL */ > > @@ -152,6 +154,7 @@ typedef struct siginfo { > > #define si_trapno _sifields._sigfault._trapno > > #endif > > #define si_addr_lsb _sifields._sigfault._addr_lsb > > +#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 c30a9c1a77b2..aeee6bb0763b 100644 > > --- a/include/uapi/asm-generic/signal-defs.h > > +++ b/include/uapi/asm-generic/signal-defs.h > > @@ -19,6 +19,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 @@ > > * should be avoided for new generic flags: 3, 4, 5, 6, 7, 8, 9, 16, 24, 25, 26. > > */ > > #define SA_UNSUPPORTED 0x00000400 > > +#define SA_XFLAGS 0x00000800 > > > > #define SA_NOMASK SA_NODEFER > > #define SA_ONESHOT SA_RESETHAND > > diff --git a/kernel/signal.c b/kernel/signal.c > > index 664a6c31137e..72182eed1b8d 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -1669,6 +1669,7 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr > > info.si_flags = flags; > > info.si_isr = isr; > > #endif > > + info.si_xflags = 0; > > return force_sig_info_to_task(&info, t); > > } > > > > @@ -1701,6 +1702,7 @@ int send_sig_fault(int sig, int code, void __user *addr > > info.si_flags = flags; > > info.si_isr = isr; > > #endif > > + info.si_xflags = 0; > > return send_sig_info(info.si_signo, &info, t); > > } > > > > @@ -1715,6 +1717,7 @@ int force_sig_mceerr(int code, void __user *addr, short lsb) > > info.si_code = code; > > info.si_addr = addr; > > info.si_addr_lsb = lsb; > > + info.si_xflags = 0; > > return force_sig_info(&info); > > } > > > > @@ -1729,6 +1732,7 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct * > > info.si_code = code; > > info.si_addr = addr; > > info.si_addr_lsb = lsb; > > + info.si_xflags = 0; > > return send_sig_info(info.si_signo, &info, t); > > } > > EXPORT_SYMBOL(send_sig_mceerr); > > @@ -1744,6 +1748,7 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper) > > info.si_addr = addr; > > info.si_lower = lower; > > info.si_upper = upper; > > + info.si_xflags = 0; > > return force_sig_info(&info); > > } > > > > @@ -1758,6 +1763,7 @@ int force_sig_pkuerr(void __user *addr, u32 pkey) > > info.si_code = SEGV_PKUERR; > > info.si_addr = addr; > > info.si_pkey = pkey; > > + info.si_xflags = 0; > > return force_sig_info(&info); > > } > > #endif > > @@ -1774,6 +1780,7 @@ int force_sig_ptrace_errno_trap(int errno, void __user *addr) > > info.si_errno = errno; > > info.si_code = TRAP_HWBKPT; > > info.si_addr = addr; > > + info.si_xflags = 0; > > return force_sig_info(&info); > > } > > > > @@ -3290,6 +3297,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > > #ifdef __ARCH_SI_TRAPNO > > to->si_trapno = from->si_trapno; > > #endif > > + to->si_xflags = from->si_xflags; > > break; > > case SIL_FAULT_MCEERR: > > to->si_addr = ptr_to_compat(from->si_addr); > > @@ -3297,6 +3305,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > > to->si_trapno = from->si_trapno; > > #endif > > to->si_addr_lsb = from->si_addr_lsb; > > + to->si_xflags = from->si_xflags; > > break; > > case SIL_FAULT_BNDERR: > > to->si_addr = ptr_to_compat(from->si_addr); > > @@ -3305,6 +3314,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > > #endif > > to->si_lower = ptr_to_compat(from->si_lower); > > to->si_upper = ptr_to_compat(from->si_upper); > > + to->si_xflags = from->si_xflags; > > break; > > case SIL_FAULT_PKUERR: > > to->si_addr = ptr_to_compat(from->si_addr); > > @@ -3312,6 +3322,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > > to->si_trapno = from->si_trapno; > > #endif > > to->si_pkey = from->si_pkey; > > + to->si_xflags = from->si_xflags; > > break; > > case SIL_CHLD: > > to->si_pid = from->si_pid; > > @@ -3370,6 +3381,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > #ifdef __ARCH_SI_TRAPNO > > to->si_trapno = from->si_trapno; > > #endif > > + to->si_xflags = from->si_xflags; > > break; > > case SIL_FAULT_MCEERR: > > to->si_addr = compat_ptr(from->si_addr); > > @@ -3377,6 +3389,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > to->si_trapno = from->si_trapno; > > #endif > > to->si_addr_lsb = from->si_addr_lsb; > > + to->si_xflags = from->si_xflags; > > break; > > case SIL_FAULT_BNDERR: > > to->si_addr = compat_ptr(from->si_addr); > > @@ -3385,6 +3398,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > #endif > > to->si_lower = compat_ptr(from->si_lower); > > to->si_upper = compat_ptr(from->si_upper); > > + to->si_xflags = from->si_xflags; > > break; > > case SIL_FAULT_PKUERR: > > to->si_addr = compat_ptr(from->si_addr); > > @@ -3392,6 +3406,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > to->si_trapno = from->si_trapno; > > #endif > > to->si_pkey = from->si_pkey; > > + to->si_xflags = from->si_xflags; > > How did you figure out the list of places to make these changes? I'm > not sure how to confirm that it's exhaustive. I looked for all places in the kernel where the si_addr field was being assigned to (basically: git grep '\bsi_addr\s*=') and added my code there. > It's a shame if we can't simply apply the change in one place. > Would the refactoring be too invasive to accomplish that? I believe that Eric mentioned that kernel/signal.c is supposed to be "the" file that knows about all of the fields of siginfo (I'm not sure what's going on in arch/powerpc, I think that's a recent regression). Maybe it's possible to eliminate the duplication between the various SIL_FAULT* cases, but aside from that it looks like there wouldn't be much duplication in the rest of the code. Peter
On Wed, Aug 19, 2020 at 06:37:25PM -0700, Peter Collingbourne wrote: > On Wed, Aug 19, 2020 at 8:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Mon, Aug 17, 2020 at 08:33:50PM -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_ignored_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. > > > > > > Ideally this field could have just been named si_flags, but that > > > name was already taken by ia64, so a different name was chosen. > > > > > > Alternatively, we may consider 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. > > > > For clarity, is the new si_xflags field supposed to be valid for all > > signal types, or just certain signals and si_codes? > > It is intended to be valid for all signal types that use the _sigfault > union member of siginfo. As listed in siginfo.h these are: SIGILL, > SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT. SIGSYS is similar to SIGILL, is that included also? > > What happens for things like a rt_sigqueueinfo() from userspace? > > Hmm. Let's enumerate each of these things, which I believe are all of > the call sites of the function copy_siginfo_from_user and related > functions (correct me if I'm wrong): > > - ptrace(PTRACE_SETSIGINFO) > - pidfd_send_signal > - rt_sigqueueinfo > - rt_tgsigqueueinfo > > We can handle the last three by observing that the kernel forbids > sending a signal with these syscalls if si_code >= 0, so we can say > that the value of si_xflags is only valid if si_code >= 0. Hmmm, that's what the code says (actually >= 0 or SI_TKILL), but it's illogical. Those are user signals, so there's no obvious reason why userspace shouldn't be allowed to generate their siginfo. It would probably be better for the kernel to police si_pid etc. in the SI_USER and SI_TKILL cases rather than flatly refusing, but I guess that's a discussion for another day. I guess the combination of SI_FROMKERNEL() and the signal number being a known fault signal if probably sufficient for now. It might be helpful to have a helper to identify fault signals, but we don't have this today, and it's unlikely that a new kind of fault signal will crop up any time soon. Handlers that handle specific signal types won't care, but debuggers and generic backtracer code would have to be hand-hacked to add new kinds of fault signal today -- not a huge priority though, and orthogonal to this series. > As for the first one, it's more tricky. Arguably something like a > debugger should be able to send arbitrary signals to a debuggee, and > there's no reason why it shouldn't be able to set si_xflags in > siginfo, but on the other hand who knows how existing debuggers end up > setting this field today. Maybe all that we can do is have the kernel > clear si_xflags if it detects that the signal uses _sigfault, and let > si_xflags aware debuggers opt out of this behavior, perhaps by > introducing a PTRACE_SETSIGINFO2 or something. Most likely a debugger usually amends an existing siginfo from a trapped signal than generating a new one from scratch. Given the other things that ptrace can do to the target process I don't think we need to police here, but your suggestion about a PTRACE_SETSIGINFO2 or similar, and zeroing this field by default with PTRACE_SETSIGINFO, does make sense. [...] > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > index 664a6c31137e..72182eed1b8d 100644 > > > --- a/kernel/signal.c > > > +++ b/kernel/signal.c [...] > > > @@ -3377,6 +3389,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > > to->si_trapno = from->si_trapno; > > > #endif > > > to->si_addr_lsb = from->si_addr_lsb; > > > + to->si_xflags = from->si_xflags; > > > break; > > > case SIL_FAULT_BNDERR: > > > to->si_addr = compat_ptr(from->si_addr); > > > @@ -3385,6 +3398,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > > #endif > > > to->si_lower = compat_ptr(from->si_lower); > > > to->si_upper = compat_ptr(from->si_upper); > > > + to->si_xflags = from->si_xflags; > > > break; > > > case SIL_FAULT_PKUERR: > > > to->si_addr = compat_ptr(from->si_addr); > > > @@ -3392,6 +3406,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > > > to->si_trapno = from->si_trapno; > > > #endif > > > to->si_pkey = from->si_pkey; > > > + to->si_xflags = from->si_xflags; > > > > How did you figure out the list of places to make these changes? I'm > > not sure how to confirm that it's exhaustive. > > I looked for all places in the kernel where the si_addr field was > being assigned to (basically: git grep '\bsi_addr\s*=') and added my > code there. > > > It's a shame if we can't simply apply the change in one place. > > Would the refactoring be too invasive to accomplish that? > > I believe that Eric mentioned that kernel/signal.c is supposed to be > "the" file that knows about all of the fields of siginfo (I'm not sure > what's going on in arch/powerpc, I think that's a recent regression). > Maybe it's possible to eliminate the duplication between the various > SIL_FAULT* cases, but aside from that it looks like there wouldn't be > much duplication in the rest of the code. If it's all contained in signal.c I guess it's not quite so bad, especially if powerpc can be brought back into the fold. Cheers ---Dave
On Mon, Aug 24, 2020 at 7:03 AM Dave Martin <Dave.Martin@arm.com> wrote: > > On Wed, Aug 19, 2020 at 06:37:25PM -0700, Peter Collingbourne wrote: > > On Wed, Aug 19, 2020 at 8:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > On Mon, Aug 17, 2020 at 08:33:50PM -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_ignored_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. > > > > > > > > Ideally this field could have just been named si_flags, but that > > > > name was already taken by ia64, so a different name was chosen. > > > > > > > > Alternatively, we may consider 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. > > > > > > For clarity, is the new si_xflags field supposed to be valid for all > > > signal types, or just certain signals and si_codes? > > > > It is intended to be valid for all signal types that use the _sigfault > > union member of siginfo. As listed in siginfo.h these are: SIGILL, > > SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT. > > SIGSYS is similar to SIGILL, is that included also? I think that SIGSYS is covered by a separate _sigsys union member. > > > What happens for things like a rt_sigqueueinfo() from userspace? > > > > Hmm. Let's enumerate each of these things, which I believe are all of > > the call sites of the function copy_siginfo_from_user and related > > functions (correct me if I'm wrong): > > > > - ptrace(PTRACE_SETSIGINFO) > > - pidfd_send_signal > > - rt_sigqueueinfo > > - rt_tgsigqueueinfo > > > > We can handle the last three by observing that the kernel forbids > > sending a signal with these syscalls if si_code >= 0, so we can say > > that the value of si_xflags is only valid if si_code >= 0. > > Hmmm, that's what the code says (actually >= 0 or SI_TKILL), but it's > illogical. Those are user signals, so there's no obvious reason why > userspace shouldn't be allowed to generate their siginfo. It would > probably be better for the kernel to police si_pid etc. in the SI_USER > and SI_TKILL cases rather than flatly refusing, but I guess that's a > discussion for another day. > > I guess the combination of SI_FROMKERNEL() and the signal number being a > known fault signal if probably sufficient for now. In v10 I ended up adding a comment saying that si_xflags is only valid if 0 <= si_code < SI_KERNEL (the SI_KERNEL part was due to my discovery of kernel code that was calling force_sig(SIGSEGV) where force_sig uses the _kill union member). Your comment about SI_USER made me realize that is not exactly true (since kill and pidfd_send_signal can send a fault signal with si_code == SI_USER). I was not aware of the SI_FROMKERNEL() macro. In v11 I will update the comment to say that SI_FROMKERNEL(si) && si->si_code != SI_KERNEL must be true in order for si_xflags to be valid. > It might be helpful to have a helper to identify fault signals, but we > don't have this today, and it's unlikely that a new kind of fault signal > will crop up any time soon. > > Handlers that handle specific signal types won't care, but debuggers and > generic backtracer code would have to be hand-hacked to add new kinds of > fault signal today -- not a huge priority though, and orthogonal to this > series. > > > As for the first one, it's more tricky. Arguably something like a > > debugger should be able to send arbitrary signals to a debuggee, and > > there's no reason why it shouldn't be able to set si_xflags in > > siginfo, but on the other hand who knows how existing debuggers end up > > setting this field today. Maybe all that we can do is have the kernel > > clear si_xflags if it detects that the signal uses _sigfault, and let > > si_xflags aware debuggers opt out of this behavior, perhaps by > > introducing a PTRACE_SETSIGINFO2 or something. > > Most likely a debugger usually amends an existing siginfo from a trapped > signal than generating a new one from scratch. Right, but it could have copied the fields by hand from a kernel-supplied siginfo before amending it. > Given the other things that ptrace can do to the target process I don't > think we need to police here, but your suggestion about a > PTRACE_SETSIGINFO2 or similar, and zeroing this field by default with > PTRACE_SETSIGINFO, does make sense. Ack. I've implemented that in v10. Peter
On Mon, Aug 24, 2020 at 06:27:51PM -0700, Peter Collingbourne wrote: > On Mon, Aug 24, 2020 at 7:03 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Wed, Aug 19, 2020 at 06:37:25PM -0700, Peter Collingbourne wrote: > > > On Wed, Aug 19, 2020 at 8:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > > > On Mon, Aug 17, 2020 at 08:33:50PM -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_ignored_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. > > > > > > > > > > Ideally this field could have just been named si_flags, but that > > > > > name was already taken by ia64, so a different name was chosen. > > > > > > > > > > Alternatively, we may consider 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. > > > > > > > > For clarity, is the new si_xflags field supposed to be valid for all > > > > signal types, or just certain signals and si_codes? > > > > > > It is intended to be valid for all signal types that use the _sigfault > > > union member of siginfo. As listed in siginfo.h these are: SIGILL, > > > SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT. > > > > SIGSYS is similar to SIGILL, is that included also? > > I think that SIGSYS is covered by a separate _sigsys union member. > > > > > What happens for things like a rt_sigqueueinfo() from userspace? > > > > > > Hmm. Let's enumerate each of these things, which I believe are all of > > > the call sites of the function copy_siginfo_from_user and related > > > functions (correct me if I'm wrong): > > > > > > - ptrace(PTRACE_SETSIGINFO) > > > - pidfd_send_signal > > > - rt_sigqueueinfo > > > - rt_tgsigqueueinfo > > > > > > We can handle the last three by observing that the kernel forbids > > > sending a signal with these syscalls if si_code >= 0, so we can say > > > that the value of si_xflags is only valid if si_code >= 0. > > > > Hmmm, that's what the code says (actually >= 0 or SI_TKILL), but it's > > illogical. Those are user signals, so there's no obvious reason why > > userspace shouldn't be allowed to generate their siginfo. It would > > probably be better for the kernel to police si_pid etc. in the SI_USER > > and SI_TKILL cases rather than flatly refusing, but I guess that's a > > discussion for another day. > > > > I guess the combination of SI_FROMKERNEL() and the signal number being a > > known fault signal if probably sufficient for now. > > In v10 I ended up adding a comment saying that si_xflags is only valid > if 0 <= si_code < SI_KERNEL (the SI_KERNEL part was due to my > discovery of kernel code that was calling force_sig(SIGSEGV) where > force_sig uses the _kill union member). Your comment about SI_USER Although it's been there a long time, is this a bug? sigaction(2) says that SI_KERNEL can be reported for any signal, but doesn't say how/why. It also says that si_addr is [unconditionally] valid for [kernel-generated] SIGSEGV. ([] are my insertions). While it may be reasonable to expect userspace code to filter out user signals before assuming that siginfo fields are value, requiring user code to check for specific si_codes is a bit nastier. I rather suspect that little or no code out there is explicity checking for SI_KERNEL before assuming that si_addr etc. are valid today. > made me realize that is not exactly true (since kill and > pidfd_send_signal can send a fault signal with si_code == SI_USER). I > was not aware of the SI_FROMKERNEL() macro. In v11 I will update the > comment to say that SI_FROMKERNEL(si) && si->si_code != SI_KERNEL must > be true in order for si_xflags to be valid. Given the above, maybe it would be better to say nothing explicit about SI_KERNEL, but make sure that the additional siginfo fields are sanely zeroed anyway. For kernel-generated signals we can guarantee this, so I think that requiring userspace to check explicitly for SI_KERNEL is too unrealistic (i.e., 90% of the time, people will forget ... and 99% of the time they will get away with it). I'm not sure that SI_FROMKERNEL() is standard btw, but I observed that it is present in the Linux UAPI header. The "si_code < 0 means userspace" convention seems well established, but I haven't found anywhere that this is clearly specified. > > > It might be helpful to have a helper to identify fault signals, but we > > don't have this today, and it's unlikely that a new kind of fault signal > > will crop up any time soon. > > > > Handlers that handle specific signal types won't care, but debuggers and > > generic backtracer code would have to be hand-hacked to add new kinds of > > fault signal today -- not a huge priority though, and orthogonal to this > > series. > > > > > As for the first one, it's more tricky. Arguably something like a > > > debugger should be able to send arbitrary signals to a debuggee, and > > > there's no reason why it shouldn't be able to set si_xflags in > > > siginfo, but on the other hand who knows how existing debuggers end up > > > setting this field today. Maybe all that we can do is have the kernel > > > clear si_xflags if it detects that the signal uses _sigfault, and let > > > si_xflags aware debuggers opt out of this behavior, perhaps by > > > introducing a PTRACE_SETSIGINFO2 or something. > > > > Most likely a debugger usually amends an existing siginfo from a trapped > > signal than generating a new one from scratch. > > Right, but it could have copied the fields by hand from a > kernel-supplied siginfo before amending it. It could have, but we can't cover every eventuality. With a PTRACE_SETSIGINFO2, we at least get a promise from userspace that it processed the fields correctly. (If userspace is wrong, or lying, that's its own funeral. The ptracer is as good as root from the point of view of the traced process anyway.) > > Given the other things that ptrace can do to the target process I don't > > think we need to police here, but your suggestion about a > > PTRACE_SETSIGINFO2 or similar, and zeroing this field by default with > > PTRACE_SETSIGINFO, does make sense. > > Ack. I've implemented that in v10. Cheers ---Dave
On Tue, Aug 25, 2020 at 7:47 AM Dave Martin <Dave.Martin@arm.com> wrote: > > On Mon, Aug 24, 2020 at 06:27:51PM -0700, Peter Collingbourne wrote: > > On Mon, Aug 24, 2020 at 7:03 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > On Wed, Aug 19, 2020 at 06:37:25PM -0700, Peter Collingbourne wrote: > > > > On Wed, Aug 19, 2020 at 8:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > > > > > On Mon, Aug 17, 2020 at 08:33:50PM -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_ignored_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. > > > > > > > > > > > > Ideally this field could have just been named si_flags, but that > > > > > > name was already taken by ia64, so a different name was chosen. > > > > > > > > > > > > Alternatively, we may consider 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. > > > > > > > > > > For clarity, is the new si_xflags field supposed to be valid for all > > > > > signal types, or just certain signals and si_codes? > > > > > > > > It is intended to be valid for all signal types that use the _sigfault > > > > union member of siginfo. As listed in siginfo.h these are: SIGILL, > > > > SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT. > > > > > > SIGSYS is similar to SIGILL, is that included also? > > > > I think that SIGSYS is covered by a separate _sigsys union member. > > > > > > > What happens for things like a rt_sigqueueinfo() from userspace? > > > > > > > > Hmm. Let's enumerate each of these things, which I believe are all of > > > > the call sites of the function copy_siginfo_from_user and related > > > > functions (correct me if I'm wrong): > > > > > > > > - ptrace(PTRACE_SETSIGINFO) > > > > - pidfd_send_signal > > > > - rt_sigqueueinfo > > > > - rt_tgsigqueueinfo > > > > > > > > We can handle the last three by observing that the kernel forbids > > > > sending a signal with these syscalls if si_code >= 0, so we can say > > > > that the value of si_xflags is only valid if si_code >= 0. > > > > > > Hmmm, that's what the code says (actually >= 0 or SI_TKILL), but it's > > > illogical. Those are user signals, so there's no obvious reason why > > > userspace shouldn't be allowed to generate their siginfo. It would > > > probably be better for the kernel to police si_pid etc. in the SI_USER > > > and SI_TKILL cases rather than flatly refusing, but I guess that's a > > > discussion for another day. > > > > > > I guess the combination of SI_FROMKERNEL() and the signal number being a > > > known fault signal if probably sufficient for now. > > > > In v10 I ended up adding a comment saying that si_xflags is only valid > > if 0 <= si_code < SI_KERNEL (the SI_KERNEL part was due to my > > discovery of kernel code that was calling force_sig(SIGSEGV) where > > force_sig uses the _kill union member). Your comment about SI_USER > > Although it's been there a long time, is this a bug? > > sigaction(2) says that SI_KERNEL can be reported for any signal, but > doesn't say how/why. It also says that si_addr is [unconditionally] > valid for [kernel-generated] SIGSEGV. ([] are my insertions). > > While it may be reasonable to expect userspace code to filter out user > signals before assuming that siginfo fields are value, requiring user > code to check for specific si_codes is a bit nastier. > > I rather suspect that little or no code out there is explicity checking > for SI_KERNEL before assuming that si_addr etc. are valid today. Right, but maybe that can be attributed to poor documentation (in the man page), so maybe the right thing to do here is to make the documentation more explicit. The kernel code itself is fairly clear that SI_KERNEL does not use the _sigfault layout: https://github.com/torvalds/linux/blob/6a9dc5fd6170d0a41c8a14eb19e63d94bea5705a/kernel/signal.c#L3173 And note that force_sig does not make the si_addr field valid either, it sets it to 0 (on most architectures, as a result of si_addr overlapping si_pid/si_uid which get set to 0 by that function), which is not necessarily the correct value. For example, on 64-bit x86, executing this code: volatile auto ptr = (char *)0xfedcba9876543210; *ptr = 42; (i.e. accessing outside of the TASK_SIZE limit) will result in a call to force_sig(SIGSEGV) setting si_addr=0. But this is clearly not an accurate fault address. I don't know how x86 reports the fault address to the kernel in this case but maybe it simply isn't available for addresses larger than TASK_SIZE, so the right thing for the kernel to do would be to indicate that the address is unavailable (for example, by setting si_code=SI_KERNEL, as it is already doing). Then through documentation updates, userspace can know that si_code=SI_KERNEL means that the address is unavailable. > > made me realize that is not exactly true (since kill and > > pidfd_send_signal can send a fault signal with si_code == SI_USER). I > > was not aware of the SI_FROMKERNEL() macro. In v11 I will update the > > comment to say that SI_FROMKERNEL(si) && si->si_code != SI_KERNEL must > > be true in order for si_xflags to be valid. > > Given the above, maybe it would be better to say nothing explicit about > SI_KERNEL, but make sure that the additional siginfo fields are sanely > zeroed anyway. I think that for si_addr this happens as a result of setting si_pid/si_uid, and for the other fields this happens as a result of zeroing the padding between fields. I know that we'd prefer not to rely on zeroing padding, but perhaps the zero padding can more be seen as a last resort for keeping things working in case userspace fails to check for SI_KERNEL. > For kernel-generated signals we can guarantee this, so I > think that requiring userspace to check explicitly for SI_KERNEL is too > unrealistic (i.e., 90% of the time, people will forget ... and 99% of > the time they will get away with it). It's unfortunate that the conditions for accessing these fields are so complex, but again this seems like part of the hand that we've been dealt with this API. Fortunately the requirement to check for SI_KERNEL should only really apply in practice to code accessing our new fields. We can make it retroactively apply to existing fields, but since that wouldn't be a change to the kernel code, just the documentation, existing code will continue to operate in the same way as it did before. > I'm not sure that SI_FROMKERNEL() is standard btw, but I observed that > it is present in the Linux UAPI header. > > The "si_code < 0 means userspace" convention seems well established, but > I haven't found anywhere that this is clearly specified. > > > > > > It might be helpful to have a helper to identify fault signals, but we > > > don't have this today, and it's unlikely that a new kind of fault signal > > > will crop up any time soon. > > > > > > Handlers that handle specific signal types won't care, but debuggers and > > > generic backtracer code would have to be hand-hacked to add new kinds of > > > fault signal today -- not a huge priority though, and orthogonal to this > > > series. > > > > > > > As for the first one, it's more tricky. Arguably something like a > > > > debugger should be able to send arbitrary signals to a debuggee, and > > > > there's no reason why it shouldn't be able to set si_xflags in > > > > siginfo, but on the other hand who knows how existing debuggers end up > > > > setting this field today. Maybe all that we can do is have the kernel > > > > clear si_xflags if it detects that the signal uses _sigfault, and let > > > > si_xflags aware debuggers opt out of this behavior, perhaps by > > > > introducing a PTRACE_SETSIGINFO2 or something. > > > > > > Most likely a debugger usually amends an existing siginfo from a trapped > > > signal than generating a new one from scratch. > > > > Right, but it could have copied the fields by hand from a > > kernel-supplied siginfo before amending it. > > It could have, but we can't cover every eventuality. With a > PTRACE_SETSIGINFO2, we at least get a promise from userspace that it > processed the fields correctly. (If userspace is wrong, or lying, > that's its own funeral. The ptracer is as good as root from the point > of view of the traced process anyway.) Agreed. Peter
On Tue, Aug 25, 2020 at 01:08:35PM -0700, Peter Collingbourne wrote: > On Tue, Aug 25, 2020 at 7:47 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Mon, Aug 24, 2020 at 06:27:51PM -0700, Peter Collingbourne wrote: > > > On Mon, Aug 24, 2020 at 7:03 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > > > On Wed, Aug 19, 2020 at 06:37:25PM -0700, Peter Collingbourne wrote: > > > > > On Wed, Aug 19, 2020 at 8:40 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > > > > > > > On Mon, Aug 17, 2020 at 08:33:50PM -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_ignored_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. > > > > > > > > > > > > > > Ideally this field could have just been named si_flags, but that > > > > > > > name was already taken by ia64, so a different name was chosen. > > > > > > > > > > > > > > Alternatively, we may consider 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. > > > > > > > > > > > > For clarity, is the new si_xflags field supposed to be valid for all > > > > > > signal types, or just certain signals and si_codes? > > > > > > > > > > It is intended to be valid for all signal types that use the _sigfault > > > > > union member of siginfo. As listed in siginfo.h these are: SIGILL, > > > > > SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT. > > > > > > > > SIGSYS is similar to SIGILL, is that included also? > > > > > > I think that SIGSYS is covered by a separate _sigsys union member. > > > > > > > > > What happens for things like a rt_sigqueueinfo() from userspace? > > > > > > > > > > Hmm. Let's enumerate each of these things, which I believe are all of > > > > > the call sites of the function copy_siginfo_from_user and related > > > > > functions (correct me if I'm wrong): > > > > > > > > > > - ptrace(PTRACE_SETSIGINFO) > > > > > - pidfd_send_signal > > > > > - rt_sigqueueinfo > > > > > - rt_tgsigqueueinfo > > > > > > > > > > We can handle the last three by observing that the kernel forbids > > > > > sending a signal with these syscalls if si_code >= 0, so we can say > > > > > that the value of si_xflags is only valid if si_code >= 0. > > > > > > > > Hmmm, that's what the code says (actually >= 0 or SI_TKILL), but it's > > > > illogical. Those are user signals, so there's no obvious reason why > > > > userspace shouldn't be allowed to generate their siginfo. It would > > > > probably be better for the kernel to police si_pid etc. in the SI_USER > > > > and SI_TKILL cases rather than flatly refusing, but I guess that's a > > > > discussion for another day. > > > > > > > > I guess the combination of SI_FROMKERNEL() and the signal number being a > > > > known fault signal if probably sufficient for now. > > > > > > In v10 I ended up adding a comment saying that si_xflags is only valid > > > if 0 <= si_code < SI_KERNEL (the SI_KERNEL part was due to my > > > discovery of kernel code that was calling force_sig(SIGSEGV) where > > > force_sig uses the _kill union member). Your comment about SI_USER > > > > Although it's been there a long time, is this a bug? > > > > sigaction(2) says that SI_KERNEL can be reported for any signal, but > > doesn't say how/why. It also says that si_addr is [unconditionally] > > valid for [kernel-generated] SIGSEGV. ([] are my insertions). > > > > While it may be reasonable to expect userspace code to filter out user > > signals before assuming that siginfo fields are value, requiring user > > code to check for specific si_codes is a bit nastier. > > > > I rather suspect that little or no code out there is explicity checking > > for SI_KERNEL before assuming that si_addr etc. are valid today. > > Right, but maybe that can be attributed to poor documentation (in the > man page), so maybe the right thing to do here is to make the > documentation more explicit. The kernel code itself is fairly clear > that SI_KERNEL does not use the _sigfault layout: > https://github.com/torvalds/linux/blob/6a9dc5fd6170d0a41c8a14eb19e63d94bea5705a/kernel/signal.c#L3173 > > And note that force_sig does not make the si_addr field valid either, > it sets it to 0 (on most architectures, as a result of si_addr > overlapping si_pid/si_uid which get set to 0 by that function), which > is not necessarily the correct value. For example, on 64-bit x86, > executing this code: > > volatile auto ptr = (char *)0xfedcba9876543210; > *ptr = 42; > > (i.e. accessing outside of the TASK_SIZE limit) will result in a call > to force_sig(SIGSEGV) setting si_addr=0. But this is clearly not an > accurate fault address. I don't know how x86 reports the fault address > to the kernel in this case but maybe it simply isn't available for > addresses larger than TASK_SIZE, so the right thing for the kernel to > do would be to indicate that the address is unavailable (for example, > by setting si_code=SI_KERNEL, as it is already doing). Then through > documentation updates, userspace can know that si_code=SI_KERNEL means > that the address is unavailable. > > > > made me realize that is not exactly true (since kill and > > > pidfd_send_signal can send a fault signal with si_code == SI_USER). I > > > was not aware of the SI_FROMKERNEL() macro. In v11 I will update the > > > comment to say that SI_FROMKERNEL(si) && si->si_code != SI_KERNEL must > > > be true in order for si_xflags to be valid. > > > > Given the above, maybe it would be better to say nothing explicit about > > SI_KERNEL, but make sure that the additional siginfo fields are sanely > > zeroed anyway. > > I think that for si_addr this happens as a result of setting > si_pid/si_uid, and for the other fields this happens as a result of > zeroing the padding between fields. I know that we'd prefer not to > rely on zeroing padding, but perhaps the zero padding can more be seen > as a last resort for keeping things working in case userspace fails to > check for SI_KERNEL. Yes, I think this is reasonable -- slightly icky, but it should work in practice. As you say, things are already a bit of a mess in this area, so the best we can reasonably aim for just now is to avoid making it the situation worse. So, can we make the condition something like: For a given signal delivery, si_xflags exists if and only if: a) the sigaction() trickery worked, _and_ b) si_addr exists. We could provide a helper macro for this test, but the condition for (b) will be fragile and tend to bitrot unless the problem is addressed more broadly. Seems out of scope for this series, though. [--- This is a digression, but: Yielding SI_KERNEL for SIGSEGV seems to be against POSIX, unless SI_KERNEL provides si_addr -- see [1]: [...] the following signal-specific information shall be available: [...] SIGSEGV, SIGBUS: void *si_addr: Address of faulting memory references. (stated unconditionally). Though, also: For some implementations, the value of si_addr may be inaccurate. IMHO, raising SIGSEGV from the kernel with an si_code that doesn't provide an si_addr (contents notwithstanding) is simply wrong, and in some of the affected cases (such as access to kernel memory) it's wrong to use non-POSIX codes at all. This is obviously just access to a duff address, so we should probably use SEGV_MAPERR for this. POSIX permits that si_addr is not always "accurate", so we could happily yield si_addr == 0 for such cases, if we don't have the address or would rather not expose it (though if the address came from userspace, it probably doesn't hurt to expose it). AFAICT, SI_KERNEL most naturally applies to situations that closely resemble kill(2) but don't originate from userspace, such as when the OOM killer kills a task. It doesn't seem right to be reporting any such thing via SIGSEGV. Anyway, this is a separate discussion from this series... -- [1] SUSv7, signal.h: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html ---] > > > For kernel-generated signals we can guarantee this, so I > > think that requiring userspace to check explicitly for SI_KERNEL is too > > unrealistic (i.e., 90% of the time, people will forget ... and 99% of > > the time they will get away with it). > > It's unfortunate that the conditions for accessing these fields are so > complex, but again this seems like part of the hand that we've been > dealt with this API. Fortunately the requirement to check for > SI_KERNEL should only really apply in practice to code accessing our > new fields. We can make it retroactively apply to existing fields, but > since that wouldn't be a change to the kernel code, just the > documentation, existing code will continue to operate in the same way > as it did before. Agreed. Again, we should try not to make things worse, but that's all. si_code stinks generally, because it's so inextensible. There's no way to add new values without leaving software utterly baffled (or more likely mistaken, since software often ignores si_code or doesn't check it exhaustively, before relying on si_* fields). [...] Cheers ---Dave
diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h index d1070a783993..6b2630dfe1df 100644 --- a/arch/arm/include/asm/signal.h +++ b/arch/arm/include/asm/signal.h @@ -19,7 +19,8 @@ typedef struct { #define SA_UAPI_FLAGS \ (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_THIRTYTWO | \ - SA_RESTORER | SA_ONSTACK | SA_RESTART | SA_NODEFER | SA_RESETHAND) + SA_RESTORER | SA_ONSTACK | SA_RESTART | SA_NODEFER | SA_RESETHAND | \ + SA_XFLAGS) #define __ARCH_HAS_SA_RESTORER diff --git a/arch/parisc/include/asm/signal.h b/arch/parisc/include/asm/signal.h index ad06e14f6e8a..3582bce44520 100644 --- a/arch/parisc/include/asm/signal.h +++ b/arch/parisc/include/asm/signal.h @@ -23,7 +23,7 @@ typedef struct { #define SA_UAPI_FLAGS \ (SA_ONSTACK | SA_RESETHAND | SA_NOCLDSTOP | SA_SIGINFO | SA_NODEFER | \ - SA_RESTART | SA_NOCLDWAIT | _SA_SIGGFAULT) + SA_RESTART | SA_NOCLDWAIT | _SA_SIGGFAULT | SA_XFLAGS) #include <asm/sigcontext.h> 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/include/linux/compat.h b/include/linux/compat.h index d38c4d7e83bd..55d4228dfd88 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -231,7 +231,9 @@ typedef struct compat_siginfo { char _dummy_pkey[__COMPAT_ADDR_BND_PKEY_PAD]; u32 _pkey; } _addr_pkey; + compat_uptr_t _pad[6]; }; + compat_uptr_t _xflags; } _sigfault; /* SIGPOLL */ diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h index e792f29b5846..cd3d08dde47a 100644 --- a/include/linux/signal_types.h +++ b/include/linux/signal_types.h @@ -72,11 +72,11 @@ struct ksignal { #ifdef SA_RESTORER #define SA_UAPI_FLAGS \ (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ - SA_NODEFER | SA_RESETHAND | SA_RESTORER) + SA_NODEFER | SA_RESETHAND | SA_RESTORER | SA_XFLAGS) #else #define SA_UAPI_FLAGS \ (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ - SA_NODEFER | SA_RESETHAND) + SA_NODEFER | SA_RESETHAND | SA_XFLAGS) #endif #endif diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h index cb3d6c267181..413d804623c0 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]; }; + unsigned long _xflags; } _sigfault; /* SIGPOLL */ @@ -152,6 +154,7 @@ typedef struct siginfo { #define si_trapno _sifields._sigfault._trapno #endif #define si_addr_lsb _sifields._sigfault._addr_lsb +#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 c30a9c1a77b2..aeee6bb0763b 100644 --- a/include/uapi/asm-generic/signal-defs.h +++ b/include/uapi/asm-generic/signal-defs.h @@ -19,6 +19,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 @@ * should be avoided for new generic flags: 3, 4, 5, 6, 7, 8, 9, 16, 24, 25, 26. */ #define SA_UNSUPPORTED 0x00000400 +#define SA_XFLAGS 0x00000800 #define SA_NOMASK SA_NODEFER #define SA_ONESHOT SA_RESETHAND diff --git a/kernel/signal.c b/kernel/signal.c index 664a6c31137e..72182eed1b8d 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1669,6 +1669,7 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr info.si_flags = flags; info.si_isr = isr; #endif + info.si_xflags = 0; return force_sig_info_to_task(&info, t); } @@ -1701,6 +1702,7 @@ int send_sig_fault(int sig, int code, void __user *addr info.si_flags = flags; info.si_isr = isr; #endif + info.si_xflags = 0; return send_sig_info(info.si_signo, &info, t); } @@ -1715,6 +1717,7 @@ int force_sig_mceerr(int code, void __user *addr, short lsb) info.si_code = code; info.si_addr = addr; info.si_addr_lsb = lsb; + info.si_xflags = 0; return force_sig_info(&info); } @@ -1729,6 +1732,7 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct * info.si_code = code; info.si_addr = addr; info.si_addr_lsb = lsb; + info.si_xflags = 0; return send_sig_info(info.si_signo, &info, t); } EXPORT_SYMBOL(send_sig_mceerr); @@ -1744,6 +1748,7 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper) info.si_addr = addr; info.si_lower = lower; info.si_upper = upper; + info.si_xflags = 0; return force_sig_info(&info); } @@ -1758,6 +1763,7 @@ int force_sig_pkuerr(void __user *addr, u32 pkey) info.si_code = SEGV_PKUERR; info.si_addr = addr; info.si_pkey = pkey; + info.si_xflags = 0; return force_sig_info(&info); } #endif @@ -1774,6 +1780,7 @@ int force_sig_ptrace_errno_trap(int errno, void __user *addr) info.si_errno = errno; info.si_code = TRAP_HWBKPT; info.si_addr = addr; + info.si_xflags = 0; return force_sig_info(&info); } @@ -3290,6 +3297,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, #ifdef __ARCH_SI_TRAPNO to->si_trapno = from->si_trapno; #endif + to->si_xflags = from->si_xflags; break; case SIL_FAULT_MCEERR: to->si_addr = ptr_to_compat(from->si_addr); @@ -3297,6 +3305,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, to->si_trapno = from->si_trapno; #endif to->si_addr_lsb = from->si_addr_lsb; + to->si_xflags = from->si_xflags; break; case SIL_FAULT_BNDERR: to->si_addr = ptr_to_compat(from->si_addr); @@ -3305,6 +3314,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, #endif to->si_lower = ptr_to_compat(from->si_lower); to->si_upper = ptr_to_compat(from->si_upper); + to->si_xflags = from->si_xflags; break; case SIL_FAULT_PKUERR: to->si_addr = ptr_to_compat(from->si_addr); @@ -3312,6 +3322,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, to->si_trapno = from->si_trapno; #endif to->si_pkey = from->si_pkey; + to->si_xflags = from->si_xflags; break; case SIL_CHLD: to->si_pid = from->si_pid; @@ -3370,6 +3381,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, #ifdef __ARCH_SI_TRAPNO to->si_trapno = from->si_trapno; #endif + to->si_xflags = from->si_xflags; break; case SIL_FAULT_MCEERR: to->si_addr = compat_ptr(from->si_addr); @@ -3377,6 +3389,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, to->si_trapno = from->si_trapno; #endif to->si_addr_lsb = from->si_addr_lsb; + to->si_xflags = from->si_xflags; break; case SIL_FAULT_BNDERR: to->si_addr = compat_ptr(from->si_addr); @@ -3385,6 +3398,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, #endif to->si_lower = compat_ptr(from->si_lower); to->si_upper = compat_ptr(from->si_upper); + to->si_xflags = from->si_xflags; break; case SIL_FAULT_PKUERR: to->si_addr = compat_ptr(from->si_addr); @@ -3392,6 +3406,7 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, to->si_trapno = from->si_trapno; #endif to->si_pkey = from->si_pkey; + to->si_xflags = from->si_xflags; break; case SIL_CHLD: to->si_pid = from->si_pid;
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_ignored_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. Ideally this field could have just been named si_flags, but that name was already taken by ia64, so a different name was chosen. Alternatively, we may consider 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. Signed-off-by: Peter Collingbourne <pcc@google.com> --- View this change in Gerrit: https://linux-review.googlesource.com/q/Ide155ce29366c3eab2a944ae4c51205982e5b8b2 arch/arm/include/asm/signal.h | 3 ++- arch/parisc/include/asm/signal.h | 2 +- arch/powerpc/platforms/powernv/vas-fault.c | 1 + include/linux/compat.h | 2 ++ include/linux/signal_types.h | 4 ++-- include/uapi/asm-generic/siginfo.h | 3 +++ include/uapi/asm-generic/signal-defs.h | 4 ++++ kernel/signal.c | 15 +++++++++++++++ 8 files changed, 30 insertions(+), 4 deletions(-)