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

[Peter Collingbourne <pcc@google.com>]

On Fri, Nov 20, 2020 at 9:44 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Peter Collingbourne <pcc@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@xmission.com>

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
>
>
> > 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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel