On Fri, Aug 21, 2020 at 10:10:17PM -0700, Peter Collingbourne wrote: > The kernel currently clears the tag bits (i.e. bits 56-63) in the fault > address exposed via siginfo.si_addr and sigcontext.fault_address. However, > the tag bits may be needed by tools in order to accurately diagnose > memory errors, such as HWASan [1] or future tools based on the Memory > Tagging Extension (MTE). > > We should not stop clearing these bits in the existing fault address > fields, because there may be existing userspace applications that are > expecting the tag bits to be cleared. Instead, create a new pair of > union fields in siginfo._sigfault, and store the tag bits of FAR_EL1 > there, together with a mask specifying which bits are valid. > > A flag is added to si_xflags to allow userspace to determine whether > the values in the fields are valid. > > [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html > > Signed-off-by: Peter Collingbourne <pcc@xxxxxxxxxx> > --- > View this change in Gerrit: https://linux-review.googlesource.com/q/Ia8876bad8c798e0a32df7c2ce1256c4771c81446 > > v10: > - rename the flag to SIXFLAG_ADDR_IGNORED_BITS > - use an arch hook to specify which bits are ignored, instead > of passing them explicitly > - while refactoring for the arch hook, noticed that my previous > patches missed a case involving cache maintenance instructions, > so expose the tag bits for that signal as well > > v9: > - make the ignored bits fields generic > - add some new dependent patches that prepare us to store the > field in such a way that userspace can detect their presence > > v8: > - rebase onto 5.8rc2 > > v7: > - switch to a new siginfo field instead of using sigcontext > - merge the patch back into one since the other patches are now > unnecessary > > v6: > - move fault address and fault code into the kernel_siginfo data structure > - split the patch in three since it was getting large and now has > generic and arch-specific parts > > v5: > - add padding to fault_addr_top_byte_context in order to ensure the correct > size and preserve sp alignment > > v4: > - expose only the tag bits in the context instead of the entire FAR_EL1 > - remove mention of the new context from the sigcontext.__reserved[] note > > v3: > - add documentation to tagged-pointers.rst > - update comments in sigcontext.h > > v2: > - revert changes to hw_breakpoint.c > - rename set_thread_esr to set_thread_far_esr > > Documentation/arm64/tagged-pointers.rst | 21 ++++++--- > arch/arm64/include/asm/exception.h | 2 +- > arch/arm64/include/asm/signal.h | 17 +++++++ > arch/arm64/include/asm/system_misc.h | 2 +- > arch/arm64/include/asm/traps.h | 6 +-- > arch/arm64/kernel/debug-monitors.c | 5 +-- > arch/arm64/kernel/entry-common.c | 2 - > arch/arm64/kernel/ptrace.c | 7 +-- > arch/arm64/kernel/sys_compat.c | 5 +-- > arch/arm64/kernel/traps.c | 29 ++++++------ > arch/arm64/mm/fault.c | 59 +++++++++++++------------ > arch/x86/kernel/signal_compat.c | 4 +- > include/linux/compat.h | 2 + > include/linux/signal.h | 8 ++++ > include/uapi/asm-generic/siginfo.h | 10 +++++ > kernel/signal.c | 14 +++++- > 16 files changed, 122 insertions(+), 71 deletions(-) > create mode 100644 arch/arm64/include/asm/signal.h > > diff --git a/Documentation/arm64/tagged-pointers.rst b/Documentation/arm64/tagged-pointers.rst > index eab4323609b9..14273160b38b 100644 > --- a/Documentation/arm64/tagged-pointers.rst > +++ b/Documentation/arm64/tagged-pointers.rst > @@ -53,12 +53,21 @@ visibility. > Preserving tags > --------------- > > -Non-zero tags are not preserved when delivering signals. This means that > -signal handlers in applications making use of tags cannot rely on the > -tag information for user virtual addresses being maintained for fields > -inside siginfo_t. One exception to this rule is for signals raised in > -response to watchpoint debug exceptions, where the tag information will > -be preserved. > +Non-zero tags are not preserved in the fault address fields > +siginfo.si_addr or sigcontext.fault_address when delivering > +signals. This means that signal handlers in applications making use > +of tags cannot rely on the tag information for user virtual addresses > +being maintained in these fields. One exception to this rule is for > +signals raised in response to watchpoint debug exceptions, where the > +tag information will be preserved. > + > +The fault address tag is preserved in the si_addr_ignored_bits field > +of siginfo, which is set for signals raised in response to data aborts > +and instruction aborts. The si_addr_ignored_bits_mask field indicates > +which bits of the field are valid. The validity of these fields is > +indicated by the SIXFLAG_ADDR_IGNORED_BITS flag in siginfo.si_xflags, > +and the validity of si_xflags in turn is indicated by the kernel > +indicating support for the sigaction.sa_flags flag SA_XFLAGS. > > The architecture prevents the use of a tagged PC, so the upper byte will > be set to a sign-extension of bit 55 on exception return. > diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h > index 7577a754d443..950d55dae948 100644 > --- a/arch/arm64/include/asm/exception.h > +++ b/arch/arm64/include/asm/exception.h > @@ -32,7 +32,7 @@ static inline u32 disr_to_esr(u64 disr) > } > > asmlinkage void enter_from_user_mode(void); > -void do_mem_abort(unsigned long addr, unsigned int esr, struct pt_regs *regs); > +void do_mem_abort(unsigned long far, unsigned int esr, struct pt_regs *regs); > void do_undefinstr(struct pt_regs *regs); > void do_bti(struct pt_regs *regs); > asmlinkage void bad_mode(struct pt_regs *regs, int reason, unsigned int esr); [...] > diff --git a/include/linux/compat.h b/include/linux/compat.h > index 55d4228dfd88..273146cf30fd 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -234,6 +234,8 @@ typedef struct compat_siginfo { > compat_uptr_t _pad[6]; > }; > compat_uptr_t _xflags; > + compat_uptr_t _addr_ignored_bits; > + compat_uptr_t _addr_ignored_bits_mask; > } _sigfault; > > /* SIGPOLL */ > diff --git a/include/linux/signal.h b/include/linux/signal.h > index 3edbf54493ee..b4c473c12a9b 100644 > --- a/include/linux/signal.h > +++ b/include/linux/signal.h > @@ -480,4 +480,12 @@ struct seq_file; > extern void render_sigset_t(struct seq_file *, const char *, sigset_t *); > #endif Can we have a comment here explaining what this helper needs to do, if defined? > +#ifndef arch_addr_ignored_bits_mask > +static inline unsigned long arch_addr_ignored_bits_mask(unsigned long sig, > + unsigned long si_code) > +{ > + return 0; > +} > +#endif > + > #endif /* _LINUX_SIGNAL_H */ > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > index 1fbd88d64f38..e314a38ce2d0 100644 > --- a/include/uapi/asm-generic/siginfo.h > +++ b/include/uapi/asm-generic/siginfo.h > @@ -94,6 +94,8 @@ union __sifields { > void *_pad[6]; > }; > unsigned long _xflags; > + unsigned long _addr_ignored_bits; > + unsigned long _addr_ignored_bits_mask; > } _sigfault; > > /* SIGPOLL */ > @@ -156,6 +158,8 @@ typedef struct siginfo { > #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_addr_ignored_bits _sifields._sigfault._addr_ignored_bits > +#define si_addr_ignored_bits_mask _sifields._sigfault._addr_ignored_bits_mask > #define si_lower _sifields._sigfault._addr_bnd._lower > #define si_upper _sifields._sigfault._addr_bnd._upper > #define si_pkey _sifields._sigfault._addr_pkey._pkey > @@ -296,6 +300,12 @@ typedef struct siginfo { > #define EMT_TAGOVF 1 /* tag overflow */ > #define NSIGEMT 1 > > +/* > + * SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT si_xflags > + */ > +#define SIXFLAG_ADDR_IGNORED_BITS 1 > +/* si_addr_ignored_bits{,_mask} fields valid */ > + I'm still uneasy about the "ignored bits" nomenclature, because the bits aren't ignored, and because at the C language level above, they generally _are_ considered part of the address. I don't have a great suggestion for a new name, though. If we just consider the si_addr_ignored_bits to be attributes that accompany the address, it might make sense to call it si_addr_attr_bits_mask or similar. That may or may not be considered to be less confusing. > /* > * sigevent definitions > * > diff --git a/kernel/signal.c b/kernel/signal.c > index 4259903b95cb..29654652d3aa 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1652,11 +1652,17 @@ void force_sigsegv(int sig) > static void set_sigfault_common_fields(struct kernel_siginfo *info, int sig, > int code, void __user *addr) > { > + unsigned long addr_long = (unsigned long)addr; > + unsigned long ignored_bits_mask = > + arch_addr_ignored_bits_mask(sig, code); > + > info->si_signo = sig; > info->si_errno = 0; > info->si_code = code; > - info->si_addr = addr; > - info->si_xflags = 0; > + info->si_addr = (void __user *)(addr_long & ~ignored_bits_mask); > + info->si_xflags = SIXFLAG_ADDR_IGNORED_BITS; > + info->si_addr_ignored_bits = addr_long & ignored_bits_mask; > + info->si_addr_ignored_bits_mask = ignored_bits_mask; Could we report the ignored bits optionally? i.e., if arch_addr_ignored_bits_mask() == 0, then we could perhaps leave SIXFLAG_ADDR_IGNORED_BITS clear in si_xflags, and just set si_addr_ignored_bits{,_mask} to zeros. I can't decide myself whether this would be a good idea or not... > } > > int force_sig_fault_to_task(int sig, int code, void __user *addr > @@ -3271,6 +3277,8 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > to->si_trapno = from->si_trapno; > #endif > to->si_xflags = from->si_xflags; > + to->si_addr_ignored_bits = from->si_addr_ignored_bits; > + to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask; What happens if we're delivering a signal to a compat process? It looks like we apply all the usual logic, but si_addr_ignored_bits and si_addr_ignored_bits_mask simply get truncated. That might be reasonable -- we don't care about bits that don't exist for compat, irrespective of what they mean for native -- but it's probably worth a comment. > } > > switch (layout) { > @@ -3347,6 +3355,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > to->si_trapno = from->si_trapno; > #endif > to->si_xflags = from->si_xflags; > + to->si_addr_ignored_bits = from->si_addr_ignored_bits; > + to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask; Otherwise, the patch looks reasonable. Cheers ---Dave
