diff mbox series

arm64: mte: Do not service syscalls after async tag fault

Message ID 20191220013639.212396-1-pcc@google.com (mailing list archive)
State New, archived
Headers show
Series arm64: mte: Do not service syscalls after async tag fault | expand

Commit Message

Peter Collingbourne Dec. 20, 2019, 1:36 a.m. UTC 
When entering the kernel after an async tag fault due to a syscall, rather
than for another reason (e.g. preemption), we don't want to service the
syscall as it may mask the tag fault. Rewind the PC to the svc instruction
in order to give a userspace signal handler an opportunity to handle the
fault and resume, and skip all other syscall processing.

Signed-off-by: Peter Collingbourne <pcc@google.com>
---
On Tue, Dec 17, 2019 at 10:01 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Dec 13, 2019 at 05:43:15PM -0800, Peter Collingbourne wrote:
> > On Wed, Dec 11, 2019 at 10:44 AM Catalin Marinas
> > <catalin.marinas@arm.com> wrote:
> > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > > index dd2cdc0d5be2..41fae64af82a 100644
> > > --- a/arch/arm64/kernel/signal.c
> > > +++ b/arch/arm64/kernel/signal.c
> > > @@ -730,6 +730,9 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> > >         regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > >         regs->pc = (unsigned long)ka->sa.sa_handler;
> > >
> > > +       /* TCO (Tag Check Override) always cleared for signal handlers */
> > > +       regs->pstate &= ~PSR_TCO_BIT;
> > > +
> > >         if (ka->sa.sa_flags & SA_RESTORER)
> > >                 sigtramp = ka->sa.sa_restorer;
> > >         else
> > > @@ -921,6 +924,11 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
> > >                         if (thread_flags & _TIF_UPROBE)
> > >                                 uprobe_notify_resume(regs);
> > >
> > > +                       if (thread_flags & _TIF_MTE_ASYNC_FAULT) {
> > > +                               clear_thread_flag(TIF_MTE_ASYNC_FAULT);
> > > +                               force_signal_inject(SIGSEGV, SEGV_MTEAERR, 0);
> >
> > In the case where the kernel is entered due to a syscall, this will
> > inject a signal, but only after servicing the syscall. This means
> > that, for example, if the syscall is exit(), the async tag check
> > failure will be silently ignored. I can reproduce the problem with the
> > program below:
> [...]
> > This patch fixes the problem for me:
> >
> > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > index 9a9d98a443fc..d0c8918dee00 100644
> > --- a/arch/arm64/kernel/syscall.c
> > +++ b/arch/arm64/kernel/syscall.c
> > @@ -94,6 +94,8 @@ static void el0_svc_common(struct pt_regs *regs, int
> > scno, int sc_nr,
> >                            const syscall_fn_t syscall_table[])
> >  {
> >         unsigned long flags = current_thread_info()->flags;
> > +       if (flags & _TIF_MTE_ASYNC_FAULT)
> > +               return;
>
> It needs a bit of thinking. This one wouldn't work if you want to handle
> the signal and resume since it would skip the SVC instruction. We'd need
> at least to do a regs->pc -= 4 and probably move it further down in this
> function.

Okay, how does this look?

Peter

 arch/arm64/kernel/syscall.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Catalin Marinas Feb. 12, 2020, 11:09 a.m. UTC | #1 
On Thu, Dec 19, 2019 at 05:36:39PM -0800, Peter Collingbourne wrote:
> When entering the kernel after an async tag fault due to a syscall, rather
> than for another reason (e.g. preemption), we don't want to service the
> syscall as it may mask the tag fault. Rewind the PC to the svc instruction
> in order to give a userspace signal handler an opportunity to handle the
> fault and resume, and skip all other syscall processing.
> 
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> ---
[...]
>  arch/arm64/kernel/syscall.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> index 9a9d98a443fc..49ea9bb47190 100644
> --- a/arch/arm64/kernel/syscall.c
> +++ b/arch/arm64/kernel/syscall.c
> @@ -95,13 +95,29 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
>  {
>  	unsigned long flags = current_thread_info()->flags;
>  
> -	regs->orig_x0 = regs->regs[0];
> -	regs->syscallno = scno;
> -
>  	cortex_a76_erratum_1463225_svc_handler();
>  	local_daif_restore(DAIF_PROCCTX);
>  	user_exit();
>  
> +#ifdef CONFIG_ARM64_MTE
> +	if (flags & _TIF_MTE_ASYNC_FAULT) {
> +		/*
> +		 * We entered the kernel after an async tag fault due to a
> +		 * syscall, rather than for another reason (e.g. preemption).
> +		 * In this case, we don't want to service the syscall as it may
> +		 * mask the tag fault. Rewind the PC to the svc instruction in
> +		 * order to give a userspace signal handler an opportunity to
> +		 * handle the fault and resume, and skip all other syscall
> +		 * processing.
> +		 */
> +		regs->pc -= 4;
> +		return;
> +	}
> +#endif
> +
> +	regs->orig_x0 = regs->regs[0];
> +	regs->syscallno = scno;

I'm slightly worried about the interaction with single-step, other
signals. It might be better if we just use the existing syscall
restarting mechanism. Untested diff below:

-------------------8<-------------------------------
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index a12c0c88d345..db25f5d6a07c 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -102,6 +102,16 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
 	local_daif_restore(DAIF_PROCCTX);
 	user_exit();
 
+	if (system_supports_mte() && (flags & _TIF_MTE_ASYNC_FAULT)) {
+		/*
+		 * Process the asynchronous tag check fault before the actual
+		 * syscall. do_notify_resume() will send a signal to userspace
+		 * before the syscall is restarted.
+		 */
+		regs->regs[0] = -ERESTARTNOINTR;
+		return;
+	}
+
 	if (has_syscall_work(flags)) {
 		/* set default errno for user-issued syscall(-1) */
 		if (scno == NO_SYSCALL)
Peter Collingbourne Feb. 18, 2020, 9:59 p.m. UTC | #2 
On Wed, Feb 12, 2020 at 3:09 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Thu, Dec 19, 2019 at 05:36:39PM -0800, Peter Collingbourne wrote:
> > When entering the kernel after an async tag fault due to a syscall, rather
> > than for another reason (e.g. preemption), we don't want to service the
> > syscall as it may mask the tag fault. Rewind the PC to the svc instruction
> > in order to give a userspace signal handler an opportunity to handle the
> > fault and resume, and skip all other syscall processing.
> >
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > ---
> [...]
> >  arch/arm64/kernel/syscall.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > index 9a9d98a443fc..49ea9bb47190 100644
> > --- a/arch/arm64/kernel/syscall.c
> > +++ b/arch/arm64/kernel/syscall.c
> > @@ -95,13 +95,29 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
> >  {
> >       unsigned long flags = current_thread_info()->flags;
> >
> > -     regs->orig_x0 = regs->regs[0];
> > -     regs->syscallno = scno;
> > -
> >       cortex_a76_erratum_1463225_svc_handler();
> >       local_daif_restore(DAIF_PROCCTX);
> >       user_exit();
> >
> > +#ifdef CONFIG_ARM64_MTE
> > +     if (flags & _TIF_MTE_ASYNC_FAULT) {
> > +             /*
> > +              * We entered the kernel after an async tag fault due to a
> > +              * syscall, rather than for another reason (e.g. preemption).
> > +              * In this case, we don't want to service the syscall as it may
> > +              * mask the tag fault. Rewind the PC to the svc instruction in
> > +              * order to give a userspace signal handler an opportunity to
> > +              * handle the fault and resume, and skip all other syscall
> > +              * processing.
> > +              */
> > +             regs->pc -= 4;
> > +             return;
> > +     }
> > +#endif
> > +
> > +     regs->orig_x0 = regs->regs[0];
> > +     regs->syscallno = scno;
>
> I'm slightly worried about the interaction with single-step, other
> signals. It might be better if we just use the existing syscall
> restarting mechanism. Untested diff below:
>
> -------------------8<-------------------------------
> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> index a12c0c88d345..db25f5d6a07c 100644
> --- a/arch/arm64/kernel/syscall.c
> +++ b/arch/arm64/kernel/syscall.c
> @@ -102,6 +102,16 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
>         local_daif_restore(DAIF_PROCCTX);
>         user_exit();
>
> +       if (system_supports_mte() && (flags & _TIF_MTE_ASYNC_FAULT)) {
> +               /*
> +                * Process the asynchronous tag check fault before the actual
> +                * syscall. do_notify_resume() will send a signal to userspace
> +                * before the syscall is restarted.
> +                */
> +               regs->regs[0] = -ERESTARTNOINTR;
> +               return;
> +       }
> +
>         if (has_syscall_work(flags)) {
>                 /* set default errno for user-issued syscall(-1) */
>                 if (scno == NO_SYSCALL)

That works for me, and I verified that my small test program as well
as some larger unit tests behave as expected.

Tested-by: Peter Collingbourne <pcc@google.com>


Peter
Catalin Marinas Feb. 19, 2020, 4:16 p.m. UTC | #3 
On Tue, Feb 18, 2020 at 01:59:34PM -0800, Peter Collingbourne wrote:
> On Wed, Feb 12, 2020 at 3:09 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Thu, Dec 19, 2019 at 05:36:39PM -0800, Peter Collingbourne wrote:
> > > When entering the kernel after an async tag fault due to a syscall, rather
> > > than for another reason (e.g. preemption), we don't want to service the
> > > syscall as it may mask the tag fault. Rewind the PC to the svc instruction
> > > in order to give a userspace signal handler an opportunity to handle the
> > > fault and resume, and skip all other syscall processing.
> > >
> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > ---
> > [...]
> > >  arch/arm64/kernel/syscall.c | 22 +++++++++++++++++++---
> > >  1 file changed, 19 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > > index 9a9d98a443fc..49ea9bb47190 100644
> > > --- a/arch/arm64/kernel/syscall.c
> > > +++ b/arch/arm64/kernel/syscall.c
> > > @@ -95,13 +95,29 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
> > >  {
> > >       unsigned long flags = current_thread_info()->flags;
> > >
> > > -     regs->orig_x0 = regs->regs[0];
> > > -     regs->syscallno = scno;
> > > -
> > >       cortex_a76_erratum_1463225_svc_handler();
> > >       local_daif_restore(DAIF_PROCCTX);
> > >       user_exit();
> > >
> > > +#ifdef CONFIG_ARM64_MTE
> > > +     if (flags & _TIF_MTE_ASYNC_FAULT) {
> > > +             /*
> > > +              * We entered the kernel after an async tag fault due to a
> > > +              * syscall, rather than for another reason (e.g. preemption).
> > > +              * In this case, we don't want to service the syscall as it may
> > > +              * mask the tag fault. Rewind the PC to the svc instruction in
> > > +              * order to give a userspace signal handler an opportunity to
> > > +              * handle the fault and resume, and skip all other syscall
> > > +              * processing.
> > > +              */
> > > +             regs->pc -= 4;
> > > +             return;
> > > +     }
> > > +#endif
> > > +
> > > +     regs->orig_x0 = regs->regs[0];
> > > +     regs->syscallno = scno;
> >
> > I'm slightly worried about the interaction with single-step, other
> > signals. It might be better if we just use the existing syscall
> > restarting mechanism. Untested diff below:
> >
> > -------------------8<-------------------------------
> > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > index a12c0c88d345..db25f5d6a07c 100644
> > --- a/arch/arm64/kernel/syscall.c
> > +++ b/arch/arm64/kernel/syscall.c
> > @@ -102,6 +102,16 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
> >         local_daif_restore(DAIF_PROCCTX);
> >         user_exit();
> >
> > +       if (system_supports_mte() && (flags & _TIF_MTE_ASYNC_FAULT)) {
> > +               /*
> > +                * Process the asynchronous tag check fault before the actual
> > +                * syscall. do_notify_resume() will send a signal to userspace
> > +                * before the syscall is restarted.
> > +                */
> > +               regs->regs[0] = -ERESTARTNOINTR;
> > +               return;
> > +       }
> > +
> >         if (has_syscall_work(flags)) {
> >                 /* set default errno for user-issued syscall(-1) */
> >                 if (scno == NO_SYSCALL)
> 
> That works for me, and I verified that my small test program as well
> as some larger unit tests behave as expected.
> 
> Tested-by: Peter Collingbourne <pcc@google.com>

Thanks Peter.
diff mbox series

Patch

diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 9a9d98a443fc..49ea9bb47190 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -95,13 +95,29 @@  static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
 {
 	unsigned long flags = current_thread_info()->flags;
 
-	regs->orig_x0 = regs->regs[0];
-	regs->syscallno = scno;
-
 	cortex_a76_erratum_1463225_svc_handler();
 	local_daif_restore(DAIF_PROCCTX);
 	user_exit();
 
+#ifdef CONFIG_ARM64_MTE
+	if (flags & _TIF_MTE_ASYNC_FAULT) {
+		/*
+		 * We entered the kernel after an async tag fault due to a
+		 * syscall, rather than for another reason (e.g. preemption).
+		 * In this case, we don't want to service the syscall as it may
+		 * mask the tag fault. Rewind the PC to the svc instruction in
+		 * order to give a userspace signal handler an opportunity to
+		 * handle the fault and resume, and skip all other syscall
+		 * processing.
+		 */
+		regs->pc -= 4;
+		return;
+	}
+#endif
+
+	regs->orig_x0 = regs->regs[0];
+	regs->syscallno = scno;
+
 	if (has_syscall_work(flags)) {
 		/* set default errno for user-issued syscall(-1) */
 		if (scno == NO_SYSCALL)