[PATCH v20] arm64: expose FAR_EL1 tag bits in siginfo

[Eric W. Biederman]

Peter Collingbourne <pcc at google.com> writes:

> 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, introduce a flag in
> sigaction.sa_flags, SA_EXPOSE_TAGBITS, and only expose the tag bits
> there if the signal handler has this flag set.
>
> [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html

For the generic bits:
Acked-by: "Eric W. Biederman" <ebiederm at xmission.com>

Some of the arm bits look wrong.  There are a couple of cases where
it looks like you are deliberately passing an untagged address into
functions that normally take tagged addresses.

It might be a good idea to have a type distinction between the two.
Perhaps "(void __user *)" vs "(unsigned long)" so that accidentally
using the wrong one generates a type error.

I don't think I am really qualified to review all of the arm details,
and I certainly don't want to be in the middle of any arm bugs this
code might introduce.  If you will split out the generic bits of this
patch I will take it.  The this whole thing can be merged into the arm
tree and you can ensure the arm bits are correct.

Eric


> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 1ee94002801f..c5375cb7763d 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -596,33 +596,35 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
>  	return 0;
>  }
>  
> -static int __kprobes do_translation_fault(unsigned long addr,
> +static int __kprobes do_translation_fault(unsigned long far,
>  					  unsigned int esr,
>  					  struct pt_regs *regs)
>  {
> +	unsigned long addr = untagged_addr(far);
> +
>  	if (is_ttbr0_addr(addr))
> -		return do_page_fault(addr, esr, regs);
> +		return do_page_fault(far, esr, regs);
>  
> -	do_bad_area(addr, esr, regs);
> +	do_bad_area(far, esr, regs);
>  	return 0;
>  }
>  
> -static int do_alignment_fault(unsigned long addr, unsigned int esr,
> +static int do_alignment_fault(unsigned long far, unsigned int esr,
>  			      struct pt_regs *regs)
>  {
> -	do_bad_area(addr, esr, regs);
> +	do_bad_area(far, esr, regs);
>  	return 0;
>  }
>  
> -static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> +static int do_bad(unsigned long far, unsigned int esr, struct pt_regs *regs)
>  {
>  	return 1; /* "fault" */
>  }
>  
> -static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> +static int do_sea(unsigned long far, unsigned int esr, struct pt_regs *regs)
>  {
>  	const struct fault_info *inf;
> -	void __user *siaddr;
> +	unsigned long siaddr;
>  
>  	inf = esr_to_fault_info(esr);
>  
> @@ -635,18 +637,23 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  	}
>  
>  	if (esr & ESR_ELx_FnV)
> -		siaddr = NULL;
> +		siaddr = 0;
>  	else
> -		siaddr  = (void __user *)addr;
> +		siaddr  = untagged_addr(far);
>  	arm64_notify_die(inf->name, regs, inf->sig, inf->code, siaddr, esr);
>
What is going on in this function?

Are you deliberately removing the tag bits?
>  	return 0;
>  }
>  
> -static int do_tag_check_fault(unsigned long addr, unsigned int esr,
> +static int do_tag_check_fault(unsigned long far, unsigned int esr,
>  			      struct pt_regs *regs)
>  {
> -	do_bad_area(addr, esr, regs);
> +	/*
> +	 * The architecture specifies that bits 63:60 of FAR_EL1 are UNKNOWN for tag
> +	 * check faults. Mask them out now so that userspace doesn't see them.
> +	 */
> +	far &= (1UL << 60) - 1;
> +	do_bad_area(far, esr, regs);
>  	return 0;
>  }
>  
> @@ -717,11 +724,12 @@ static const struct fault_info fault_info[] = {
>  	{ do_bad,		SIGKILL, SI_KERNEL,	"unknown 63"			},
>  };
>  
> -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)
>  {
>  	const struct fault_info *inf = esr_to_fault_info(esr);
> +	unsigned long addr = untagged_addr(far);
>  
> -	if (!inf->fn(addr, esr, regs))
> +	if (!inf->fn(far, esr, regs))
>  		return;
>  
>  	if (!user_mode(regs)) {
> @@ -730,8 +738,7 @@ void do_mem_abort(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  		show_pte(addr);
>  	}
>  
> -	arm64_notify_die(inf->name, regs,
> -			 inf->sig, inf->code, (void __user *)addr, esr);
> +	arm64_notify_die(inf->name, regs, inf->sig, inf->code, addr, esr);

What is going on in this function?

Are you deliberately removing the tag bits?
>  }
>  NOKPROBE_SYMBOL(do_mem_abort);
>