Subject: Re: [PATCH v10 7/7] arm64: expose FAR_EL1 tag bits in siginfo

From: Dave Martin <[email-masked]>
Date: Tue, 8 Sep 2020 16:13:47 +0100

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 <[email-masked]>
> ---
> 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

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel