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

On Fri, Nov 20, 2020 at 9:44 AM Eric W. Biederman [off-list ref] wrote:

Peter Collingbourne [off-list ref] writes:

quoted

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" <redacted>

Thanks for the review.

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

Okay, I'll do that.

tree and you can ensure the arm bits are correct.

So you want Catalin to merge your signal-for-v5.11 branch with the
generic patches into the arm64 tree and then add the arm64-specific
patch on top? Works for me.

Eric

quoted

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?

Yes, this function handles synchronous external aborts, and the
architecture defines the tag bits as UNKNOWN for these types of
aborts. This is similar to the tag check fault case where bits 63:60
are UNKNOWN.

quoted

      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?

This part of the function handles the case where the type of the abort
is unrecognized. In this case we conservatively remove the tag bits
since they may have been defined as UNKNOWN.

Peter