[23/25] powerpc: Deliver SEGV signal on pkey violation

Message ID 1504910713-7094-32-git-send-email-linuxram@us.ibm.com
State Changes Requested
Headers show
Series
  • powerpc: Free up RPAGE_RSV bits
Related show

Commit Message

Ram Pai Sept. 8, 2017, 10:45 p.m.
The value of the pkey, whose protection got violated,
is made available in si_pkey field of the siginfo structure.

Also keep the thread's pkey-register fields up2date.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/kernel/traps.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

Comments

Michael Ellerman Oct. 24, 2017, 3:46 p.m. | #1
Ram Pai <linuxram@us.ibm.com> writes:

> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index ec74e20..f2a310d 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -265,6 +266,15 @@ void user_single_step_siginfo(struct task_struct *tsk,
>  	info->si_addr = (void __user *)regs->nip;
>  }
>  
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> +static void fill_sig_info_pkey(int si_code, siginfo_t *info, unsigned long addr)
> +{
> +	if (info->si_signo != SIGSEGV || si_code != SEGV_PKUERR)

Just checking si_code is sufficient there I think.

> +		return;
> +	info->si_pkey = get_paca()->paca_pkey;
> +}
> +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */

This should define an empty version in the #else case, so we don't need
the ifdef below.

> @@ -292,6 +302,18 @@ void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr)
>  	info.si_signo = signr;
>  	info.si_code = code;
>  	info.si_addr = (void __user *) addr;
> +
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> +	/*
> +	 * update the thread's pkey related fields.
> +	 * core-dump handlers and other sub-systems
> +	 * depend on those values.
> +	 */
> +	thread_pkey_regs_save(&current->thread);

You shouldn't need to do this.

We're not putting any of the pkey regs in the signal frame, so you don't
need to save before we do that. [And if you did the right place to do it
would be in setup_sigcontext() (or the TM version).]

For ptrace and coredumps it should happen in pkey_get(), see eg.
fpr_get() which does flush_fp_to_thread() as an example.

> +	/* update the violated-key value */
> +	fill_sig_info_pkey(code, &info, addr);
> +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */

> +
>  	force_sig_info(signr, &info, current);
>  }

cheers
Ram Pai Oct. 24, 2017, 5:19 p.m. | #2
On Tue, Oct 24, 2017 at 05:46:38PM +0200, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index ec74e20..f2a310d 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -265,6 +266,15 @@ void user_single_step_siginfo(struct task_struct *tsk,
> >  	info->si_addr = (void __user *)regs->nip;
> >  }
> >  
> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > +static void fill_sig_info_pkey(int si_code, siginfo_t *info, unsigned long addr)
> > +{
> > +	if (info->si_signo != SIGSEGV || si_code != SEGV_PKUERR)
> 
> Just checking si_code is sufficient there I think.
> 
> > +		return;
> > +	info->si_pkey = get_paca()->paca_pkey;
> > +}
> > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> 
> This should define an empty version in the #else case, so we don't need
> the ifdef below.
> 
> > @@ -292,6 +302,18 @@ void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr)
> >  	info.si_signo = signr;
> >  	info.si_code = code;
> >  	info.si_addr = (void __user *) addr;
> > +
> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > +	/*
> > +	 * update the thread's pkey related fields.
> > +	 * core-dump handlers and other sub-systems
> > +	 * depend on those values.
> > +	 */
> > +	thread_pkey_regs_save(&current->thread);
> 
> You shouldn't need to do this.
> 
> We're not putting any of the pkey regs in the signal frame, so you don't
> need to save before we do that. [And if you did the right place to do it
> would be in setup_sigcontext() (or the TM version).]

One of the comments in the past was the ability to capture AMR
register status in core-dumps and also the ability to modify the
AMR through ptrace. Thiago wrote a patch towards that. That patch
is one of the subsequent patches sent in this series.

I will move the code to pkey_get().

> 
> For ptrace and coredumps it should happen in pkey_get(), see eg.
> fpr_get() which does flush_fp_to_thread() as an example.

Ok.

> 
> > +	/* update the violated-key value */
> > +	fill_sig_info_pkey(code, &info, addr);
> > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> 
> > +
> >  	force_sig_info(signr, &info, current);
> >  }
> 
> cheers

Patch

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index ec74e20..f2a310d 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -20,6 +20,7 @@ 
 #include <linux/sched/debug.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/pkeys.h>
 #include <linux/stddef.h>
 #include <linux/unistd.h>
 #include <linux/ptrace.h>
@@ -265,6 +266,15 @@  void user_single_step_siginfo(struct task_struct *tsk,
 	info->si_addr = (void __user *)regs->nip;
 }
 
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+static void fill_sig_info_pkey(int si_code, siginfo_t *info, unsigned long addr)
+{
+	if (info->si_signo != SIGSEGV || si_code != SEGV_PKUERR)
+		return;
+	info->si_pkey = get_paca()->paca_pkey;
+}
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
+
 void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr)
 {
 	siginfo_t info;
@@ -292,6 +302,18 @@  void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr)
 	info.si_signo = signr;
 	info.si_code = code;
 	info.si_addr = (void __user *) addr;
+
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+	/*
+	 * update the thread's pkey related fields.
+	 * core-dump handlers and other sub-systems
+	 * depend on those values.
+	 */
+	thread_pkey_regs_save(&current->thread);
+	/* update the violated-key value */
+	fill_sig_info_pkey(code, &info, addr);
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
+
 	force_sig_info(signr, &info, current);
 }