Re: [PATCH] arm64: Expose original FAR_EL1 value in sigcontext

From: Peter Collingbourne <pcc@google.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>,
	Kevin Brodsky <kevin.brodsky@arm.com>,
	Kostya Serebryany <kcc@google.com>,
	Evgenii Stepanov <eugenis@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Will Deacon <will@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH] arm64: Expose original FAR_EL1 value in sigcontext
Date: Wed, 25 Mar 2020 10:41:34 -0700	[thread overview]
Message-ID: <CAMn1gO6nq1P2Z1byYWdO44WRoxhPX6FxyOS3eYc6G2BZ7yKJ2w@mail.gmail.com> (raw)
In-Reply-To: <20200325131023.GN3901@mbp>

On Wed, Mar 25, 2020 at 6:10 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> Hi Peter,
>
> On Thu, Mar 12, 2020 at 10:17:55AM -0700, Peter Collingbourne wrote:
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index fde59981445ca..290ea59c68b85 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -22,7 +22,6 @@ static void notrace el1_abort(struct pt_regs *regs, unsigned long esr)
> >       unsigned long far = read_sysreg(far_el1);
> >
> >       local_daif_inherit(regs);
> > -     far = untagged_addr(far);
> >       do_mem_abort(far, esr, regs);
> >  }
> >  NOKPROBE_SYMBOL(el1_abort);
>
> Would we get a signal on faults triggered by the kernel? Anyway, I'm
> fine with this change for consistency and may help with the fault
> information printed by the kernel with khwasan or (later) MTE.

It doesn't look like we would. As far as I can tell all of the signal
injection paths are guarded with if (user_mode(regs)) and such. Agreed
with the consistency argument.

> > @@ -104,7 +103,6 @@ static void notrace el0_da(struct pt_regs *regs, unsigned long esr)
> >
> >       user_exit_irqoff();
> >       local_daif_restore(DAIF_PROCCTX);
> > -     far = untagged_addr(far);
> >       do_mem_abort(far, esr, regs);
> >  }
> >  NOKPROBE_SYMBOL(el0_da);
> > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> > index 0b727edf41046..985cd44decf62 100644
> > --- a/arch/arm64/kernel/hw_breakpoint.c
> > +++ b/arch/arm64/kernel/hw_breakpoint.c
> > @@ -730,7 +730,7 @@ static u64 get_distance_from_watchpoint(unsigned long addr, u64 val,
> >               return 0;
> >  }
> >
> > -static int watchpoint_handler(unsigned long addr, unsigned int esr,
> > +static int watchpoint_handler(unsigned long far, unsigned int esr,
> >                             struct pt_regs *regs)
> >  {
> >       int i, step = 0, *kernel_step, access, closest_match = 0;
> > @@ -741,6 +741,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
> >       struct debug_info *debug_info;
> >       struct arch_hw_breakpoint *info;
> >       struct arch_hw_breakpoint_ctrl ctrl;
> > +     unsigned long addr = untagged_addr(far);
> >
> >       slots = this_cpu_ptr(wp_on_reg);
> >       debug_info = &current->thread.debug;
>
> Why do we need to untag this here? Have you hit any bug? This function
> gets the original FAR_EL1 value, untagged (via elX_dbg()), and we clear
> the tag further down in get_distance_from_watchpoint().

You're right, I missed that this was going via elX_dbg() rather than
an abort handler. In fact, this would seem to be a potential userspace
break because the now-untagged address is also stored in
counter_arch_bp(wp)->trigger, which is exposed to userspace via
ptrace_hbptriggered in arch/arm64/kernel/ptrace.c. I've reverted this
part in v2.

> > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > index 339882db5a915..48e8b6c7b5369 100644
> > --- a/arch/arm64/kernel/signal.c
> > +++ b/arch/arm64/kernel/signal.c
> > @@ -55,6 +55,7 @@ struct rt_sigframe_user_layout {
> >
> >       unsigned long fpsimd_offset;
> >       unsigned long esr_offset;
> > +     unsigned long far_offset;
> >       unsigned long sve_offset;
> >       unsigned long extra_offset;
> >       unsigned long end_offset;
> > @@ -383,6 +384,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
> >                       break;
> >
> >               case ESR_MAGIC:
> > +             case FAR_MAGIC:
> >                       /* ignore */
> >                       break;
> >
> > @@ -581,6 +583,11 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user,
> >                                    sizeof(struct esr_context));
> >               if (err)
> >                       return err;
> > +
> > +             err = sigframe_alloc(user, &user->far_offset,
> > +                                  sizeof(struct far_context));
> > +             if (err)
> > +                     return err;
>
> It looks fine, I think it makes sense to only expose the raw FAR_EL1
> when we also expose the ESR_EL1 (via set_thread_esr()).
>
> > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > index 85566d32958f5..2ca2de1ff43be 100644
> > --- a/arch/arm64/mm/fault.c
> > +++ b/arch/arm64/mm/fault.c
> > @@ -41,7 +41,7 @@
> >  #include <asm/traps.h>
> >
> >  struct fault_info {
> > -     int     (*fn)(unsigned long addr, unsigned int esr,
> > +     int     (*fn)(unsigned long far, unsigned int esr,
> >                     struct pt_regs *regs);
> >       int     sig;
> >       int     code;
> > @@ -320,9 +320,11 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,
> >       die_kernel_fault(msg, addr, esr, regs);
> >  }
> >
> > -static void set_thread_esr(unsigned long address, unsigned int esr)
> > +static void set_thread_esr(unsigned long far, unsigned int esr)
>
> We might as well rename this to set_thread_far_esr().

Done in v2.

Peter

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