Peter Collingbourne <pcc@xxxxxxxxxx> 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@xxxxxxxxxxxx> 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); >
