Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-31 Thread Thomas Gleixner 
On Fri, 31 Oct 2014, Dave Hansen wrote:

> On 10/27/2014 01:36 PM, Thomas Gleixner wrote:
> > You're repeating yourself. Care to read the discussion about this from
> > the last round of review again?
> 
> OK, so here's a rewritten decoder.  I think it's a lot more robust and
> probably fixes a bug or two.  This ends up saving ~70 lines of code out
> of ~300 or so for the old patch.
> 
> I'll include this in the next series, but I'm posting it early and often
> to make sure I'm on the right track.

Had a short glance. This looks really very well done!

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-31 Thread Dave Hansen 
On 10/27/2014 01:36 PM, Thomas Gleixner wrote:
> You're repeating yourself. Care to read the discussion about this from
> the last round of review again?

OK, so here's a rewritten decoder.  I think it's a lot more robust and
probably fixes a bug or two.  This ends up saving ~70 lines of code out
of ~300 or so for the old patch.

I'll include this in the next series, but I'm posting it early and often
to make sure I'm on the right track.

There is also a preparatory patch or two, but they're small.

This patch sets bound violation fields of siginfo struct in #BR
exception handler by decoding the user instruction and constructing
the faulting pointer.

We have to be very careful when decoding these instructions.  They
are completely controlled by userspace and may be changed at any
time up to and including the point where we try to copy them in to
the kernel.  They may or may not be MPX instructions and could be
completely invalid for all we know.

Note: This code is based on Qiaowei Ren's specialized MPX
decoder, but uses the generic decoder whenever possible.  It was
tested for robustness by generating a completely random data
stream and trying to decode that stream.  I also unmapped random
pages inside the stream to test the "partial instruction" short
read code.

We kzalloc() the siginfo instead of stack allocating it because
we need to memset() it anyway, and doing this makes it much more
clear when it got initialized by the MPX instruction decoder.

Changes from the old decoder:
 * Use the generic decoder instead of custom functions.  Saved
   ~70 lines of code overall.
 * Remove insn->addr_bytes code (never used??)
 * Make sure never to possibly overflow the regoff[] array, plus
   check the register range correctly in 32 and 64-bit modes.
 * Allow get_reg() to return an error and have mpx_get_addr_ref()
   handle when it sees errors.
 * Only call insn_get_*() near where we actually use the values
   instead if trying to call them all at once.
 * Handle short reads from copy_from_user() and check the actual
   number of read bytes against what we expect from
   insn_get_length().  If a read stops in the middle of an
   instruction, we error out.
 * Actually check the opcodes intead of ignoring them.
 * Dynamically kzalloc() siginfo_t so we don't leak any stack
   data.
 * Detect and handle decoder failures instead of ignoring them.


Signed-off-by: Qiaowei Ren 
Signed-off-by: Dave Hansen 
---

 b/arch/x86/include/asm/mpx.h |8 +
 b/arch/x86/kernel/mpx.c  |  229 +++
 b/arch/x86/kernel/traps.c|   19 +++
 3 files changed, 255 insertions(+), 1 deletion(-)

diff -puN arch/x86/include/asm/mpx.h~mpx-new-decoder arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~mpx-new-decoder	2014-10-31 11:36:17.130106540 -0700
+++ b/arch/x86/include/asm/mpx.h	2014-10-31 12:51:19.518584587 -0700
@@ -3,6 +3,7 @@
 
 #include 
 #include 
+#include 
 
 /*
  * NULL is theoretically a valid place to put the bounds
@@ -59,11 +60,18 @@ unsigned long mpx_mmap(unsigned long len
 
 #ifdef CONFIG_X86_INTEL_MPX
 int do_mpx_bt_fault(struct xsave_struct *xsave_buf);
+siginfo_t *mpx_generate_signifo(struct pt_regs *regs,
+struct xsave_struct *xsave_buf);
 #else
 static inline int do_mpx_bt_fault(struct xsave_struct *xsave_buf)
 {
 	return -EINVAL;
 }
+siginfo_t *mpx_generate_signifo(struct pt_regs *regs,
+struct xsave_struct *xsave_buf);
+{
+	return NULL;
+}
 #endif /* CONFIG_X86_INTEL_MPX */
 
 #endif /* _ASM_X86_MPX_H */
diff -puN arch/x86/kernel/mpx.c~mpx-new-decoder arch/x86/kernel/mpx.c
--- a/arch/x86/kernel/mpx.c~mpx-new-decoder	2014-10-31 11:36:17.131106585 -0700
+++ b/arch/x86/kernel/mpx.c	2014-10-31 13:12:22.644915144 -0700
@@ -81,6 +81,163 @@ int mpx_disable_management(struct task_s
 	return 0;
 }
 
+enum reg_type {
+	REG_TYPE_RM = 0,
+	REG_TYPE_INDEX,
+	REG_TYPE_BASE,
+};
+
+static unsigned long get_reg_offset(struct insn *insn, struct pt_regs *regs,
+enum reg_type type)
+{
+	int regno = 0;
+
+	static const int regoff[] = {
+		offsetof(struct pt_regs, ax),
+		offsetof(struct pt_regs, cx),
+		offsetof(struct pt_regs, dx),
+		offsetof(struct pt_regs, bx),
+		offsetof(struct pt_regs, sp),
+		offsetof(struct pt_regs, bp),
+		offsetof(struct pt_regs, si),
+		offsetof(struct pt_regs, di),
+#ifdef CONFIG_X86_64
+		offsetof(struct pt_regs, r8),
+		offsetof(struct pt_regs, r9),
+		offsetof(struct pt_regs, r10),
+		offsetof(struct pt_regs, r11),
+		offsetof(struct pt_regs, r12),
+		offsetof(struct pt_regs, r13),
+		offsetof(struct pt_regs, r14),
+		offsetof(struct pt_regs, r15),
+#endif
+	};
+	int nr_registers = ARRAY_SIZE(regoff);
+	/*
+	 * Don't possibly decode a 32-bit instructions as
+	 * reading a 64-bit-only register.
+	 */
+	if (IS_ENABLED(CONFIG_X86_64) && !insn->x86_64)
+		nr_registers -= 8;
+
+	switch (type) {
+	case REG_TYPE_RM:
+		regno = X86_MODRM_RM(insn->modrm.value);
+		if (X86_REX_B(insn->rex_prefix.value) == 1)
+			regno += 8;
+		break;
+
+

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-31 Thread Thomas Gleixner 
On Fri, 31 Oct 2014, Ren Qiaowei wrote:
> On 10/31/2014 06:38 AM, Dave Hansen wrote:
> > > @@ -316,6 +317,11 @@ dotraplinkage void do_bounds(struct pt_regs *regs,
> > > long error_code)
> > >   break;
> > > 
> > >   case 1: /* Bound violation. */
> > > + do_mpx_bounds(regs, , xsave_buf);
> > > + do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs,
> > > + error_code, );
> > > + break;
> > > +
> > >   case 0: /* No exception caused by Intel MPX operations. */
> > >   do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, 
> > > error_code,
> > > NULL);
> > >   break;
> > > 
> > 
> > So, siginfo is stack-allocarted here.  do_mpx_bounds() can error out if
> > it sees an invalid bndregno.  We still send the signal with the 
> > whether or not we filled the 'info' in do_mpx_bounds().
> > 
> > Can't this leak some kernel stack out in the 'info'?
> > 
> 
> This should check the return value of do_mpx_bounds and should be fixed.

And how's that answering Dave's question about leaking stack information? 

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-31 Thread Thomas Gleixner 
On Fri, 31 Oct 2014, Ren Qiaowei wrote:
 On 10/31/2014 06:38 AM, Dave Hansen wrote:
   @@ -316,6 +317,11 @@ dotraplinkage void do_bounds(struct pt_regs *regs,
   long error_code)
 break;
   
 case 1: /* Bound violation. */
   + do_mpx_bounds(regs, info, xsave_buf);
   + do_trap(X86_TRAP_BR, SIGSEGV, bounds, regs,
   + error_code, info);
   + break;
   +
 case 0: /* No exception caused by Intel MPX operations. */
 do_trap(X86_TRAP_BR, SIGSEGV, bounds, regs, 
   error_code,
   NULL);
 break;
   
  
  So, siginfo is stack-allocarted here.  do_mpx_bounds() can error out if
  it sees an invalid bndregno.  We still send the signal with the info
  whether or not we filled the 'info' in do_mpx_bounds().
  
  Can't this leak some kernel stack out in the 'info'?
  
 
 This should check the return value of do_mpx_bounds and should be fixed.

And how's that answering Dave's question about leaking stack information? 

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-31 Thread Dave Hansen 
On 10/27/2014 01:36 PM, Thomas Gleixner wrote:
 You're repeating yourself. Care to read the discussion about this from
 the last round of review again?

OK, so here's a rewritten decoder.  I think it's a lot more robust and
probably fixes a bug or two.  This ends up saving ~70 lines of code out
of ~300 or so for the old patch.

I'll include this in the next series, but I'm posting it early and often
to make sure I'm on the right track.

There is also a preparatory patch or two, but they're small.

This patch sets bound violation fields of siginfo struct in #BR
exception handler by decoding the user instruction and constructing
the faulting pointer.

We have to be very careful when decoding these instructions.  They
are completely controlled by userspace and may be changed at any
time up to and including the point where we try to copy them in to
the kernel.  They may or may not be MPX instructions and could be
completely invalid for all we know.

Note: This code is based on Qiaowei Ren's specialized MPX
decoder, but uses the generic decoder whenever possible.  It was
tested for robustness by generating a completely random data
stream and trying to decode that stream.  I also unmapped random
pages inside the stream to test the partial instruction short
read code.

We kzalloc() the siginfo instead of stack allocating it because
we need to memset() it anyway, and doing this makes it much more
clear when it got initialized by the MPX instruction decoder.

Changes from the old decoder:
 * Use the generic decoder instead of custom functions.  Saved
   ~70 lines of code overall.
 * Remove insn-addr_bytes code (never used??)
 * Make sure never to possibly overflow the regoff[] array, plus
   check the register range correctly in 32 and 64-bit modes.
 * Allow get_reg() to return an error and have mpx_get_addr_ref()
   handle when it sees errors.
 * Only call insn_get_*() near where we actually use the values
   instead if trying to call them all at once.
 * Handle short reads from copy_from_user() and check the actual
   number of read bytes against what we expect from
   insn_get_length().  If a read stops in the middle of an
   instruction, we error out.
 * Actually check the opcodes intead of ignoring them.
 * Dynamically kzalloc() siginfo_t so we don't leak any stack
   data.
 * Detect and handle decoder failures instead of ignoring them.


Signed-off-by: Qiaowei Ren qiaowei@intel.com
Signed-off-by: Dave Hansen dave.han...@intel.com
---

 b/arch/x86/include/asm/mpx.h |8 +
 b/arch/x86/kernel/mpx.c  |  229 +++
 b/arch/x86/kernel/traps.c|   19 +++
 3 files changed, 255 insertions(+), 1 deletion(-)

diff -puN arch/x86/include/asm/mpx.h~mpx-new-decoder arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~mpx-new-decoder	2014-10-31 11:36:17.130106540 -0700
+++ b/arch/x86/include/asm/mpx.h	2014-10-31 12:51:19.518584587 -0700
@@ -3,6 +3,7 @@
 
 #include linux/types.h
 #include asm/ptrace.h
+#include asm/insn.h
 
 /*
  * NULL is theoretically a valid place to put the bounds
@@ -59,11 +60,18 @@ unsigned long mpx_mmap(unsigned long len
 
 #ifdef CONFIG_X86_INTEL_MPX
 int do_mpx_bt_fault(struct xsave_struct *xsave_buf);
+siginfo_t *mpx_generate_signifo(struct pt_regs *regs,
+struct xsave_struct *xsave_buf);
 #else
 static inline int do_mpx_bt_fault(struct xsave_struct *xsave_buf)
 {
 	return -EINVAL;
 }
+siginfo_t *mpx_generate_signifo(struct pt_regs *regs,
+struct xsave_struct *xsave_buf);
+{
+	return NULL;
+}
 #endif /* CONFIG_X86_INTEL_MPX */
 
 #endif /* _ASM_X86_MPX_H */
diff -puN arch/x86/kernel/mpx.c~mpx-new-decoder arch/x86/kernel/mpx.c
--- a/arch/x86/kernel/mpx.c~mpx-new-decoder	2014-10-31 11:36:17.131106585 -0700
+++ b/arch/x86/kernel/mpx.c	2014-10-31 13:12:22.644915144 -0700
@@ -81,6 +81,163 @@ int mpx_disable_management(struct task_s
 	return 0;
 }
 
+enum reg_type {
+	REG_TYPE_RM = 0,
+	REG_TYPE_INDEX,
+	REG_TYPE_BASE,
+};
+
+static unsigned long get_reg_offset(struct insn *insn, struct pt_regs *regs,
+enum reg_type type)
+{
+	int regno = 0;
+
+	static const int regoff[] = {
+		offsetof(struct pt_regs, ax),
+		offsetof(struct pt_regs, cx),
+		offsetof(struct pt_regs, dx),
+		offsetof(struct pt_regs, bx),
+		offsetof(struct pt_regs, sp),
+		offsetof(struct pt_regs, bp),
+		offsetof(struct pt_regs, si),
+		offsetof(struct pt_regs, di),
+#ifdef CONFIG_X86_64
+		offsetof(struct pt_regs, r8),
+		offsetof(struct pt_regs, r9),
+		offsetof(struct pt_regs, r10),
+		offsetof(struct pt_regs, r11),
+		offsetof(struct pt_regs, r12),
+		offsetof(struct pt_regs, r13),
+		offsetof(struct pt_regs, r14),
+		offsetof(struct pt_regs, r15),
+#endif
+	};
+	int nr_registers = ARRAY_SIZE(regoff);
+	/*
+	 * Don't possibly decode a 32-bit instructions as
+	 * reading a 64-bit-only register.
+	 */
+	if (IS_ENABLED(CONFIG_X86_64)  !insn-x86_64)
+		nr_registers -= 8;
+
+	switch (type) {
+	case REG_TYPE_RM:
+		regno = X86_MODRM_RM(insn-modrm.value);
+		if

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-31 Thread Thomas Gleixner 
On Fri, 31 Oct 2014, Dave Hansen wrote:

 On 10/27/2014 01:36 PM, Thomas Gleixner wrote:
  You're repeating yourself. Care to read the discussion about this from
  the last round of review again?
 
 OK, so here's a rewritten decoder.  I think it's a lot more robust and
 probably fixes a bug or two.  This ends up saving ~70 lines of code out
 of ~300 or so for the old patch.
 
 I'll include this in the next series, but I'm posting it early and often
 to make sure I'm on the right track.

Had a short glance. This looks really very well done!

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-30 Thread Ren Qiaowei 

On 10/31/2014 06:38 AM, Dave Hansen wrote:

+void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
+   struct xsave_struct *xsave_buf)
+{
+   struct mpx_insn insn;
+   uint8_t bndregno;
+   unsigned long addr_vio;
+
+   addr_vio = mpx_insn_decode(, regs);
+
+   bndregno = X86_MODRM_REG(insn.modrm.value);
+   if (bndregno > 3)
+   return;
+
+   /* Note: the upper 32 bits are ignored in 32-bit mode. */
+   info->si_lower = (void __user *)(unsigned long)
+   (xsave_buf->bndregs.bndregs[2*bndregno]);
+   info->si_upper = (void __user *)(unsigned long)
+   (~xsave_buf->bndregs.bndregs[2*bndregno+1]);
+   info->si_addr_lsb = 0;
+   info->si_signo = SIGSEGV;
+   info->si_errno = 0;
+   info->si_code = SEGV_BNDERR;
+   info->si_addr = (void __user *)addr_vio;
+}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 611b6ec..b2a916b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -284,6 +284,7 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
error_code)
unsigned long status;
struct xsave_struct *xsave_buf;
struct task_struct *tsk = current;
+   siginfo_t info;

prev_state = exception_enter();
if (notify_die(DIE_TRAP, "bounds", regs, error_code,
@@ -316,6 +317,11 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
error_code)
break;

case 1: /* Bound violation. */
+   do_mpx_bounds(regs, , xsave_buf);
+   do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs,
+   error_code, );
+   break;
+
case 0: /* No exception caused by Intel MPX operations. */
do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, NULL);
break;



So, siginfo is stack-allocarted here.  do_mpx_bounds() can error out if
it sees an invalid bndregno.  We still send the signal with the 
whether or not we filled the 'info' in do_mpx_bounds().

Can't this leak some kernel stack out in the 'info'?



This should check the return value of do_mpx_bounds and should be fixed.

Thanks,
Qiaowei

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-30 Thread Dave Hansen 
> +void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
> + struct xsave_struct *xsave_buf)
> +{
> + struct mpx_insn insn;
> + uint8_t bndregno;
> + unsigned long addr_vio;
> +
> + addr_vio = mpx_insn_decode(, regs);
> +
> + bndregno = X86_MODRM_REG(insn.modrm.value);
> + if (bndregno > 3)
> + return;
> +
> + /* Note: the upper 32 bits are ignored in 32-bit mode. */
> + info->si_lower = (void __user *)(unsigned long)
> + (xsave_buf->bndregs.bndregs[2*bndregno]);
> + info->si_upper = (void __user *)(unsigned long)
> + (~xsave_buf->bndregs.bndregs[2*bndregno+1]);
> + info->si_addr_lsb = 0;
> + info->si_signo = SIGSEGV;
> + info->si_errno = 0;
> + info->si_code = SEGV_BNDERR;
> + info->si_addr = (void __user *)addr_vio;
> +}
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 611b6ec..b2a916b 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -284,6 +284,7 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
> error_code)
>   unsigned long status;
>   struct xsave_struct *xsave_buf;
>   struct task_struct *tsk = current;
> + siginfo_t info;
>  
>   prev_state = exception_enter();
>   if (notify_die(DIE_TRAP, "bounds", regs, error_code,
> @@ -316,6 +317,11 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
> error_code)
>   break;
>  
>   case 1: /* Bound violation. */
> + do_mpx_bounds(regs, , xsave_buf);
> + do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs,
> + error_code, );
> + break;
> +
>   case 0: /* No exception caused by Intel MPX operations. */
>   do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, NULL);
>   break;
> 

So, siginfo is stack-allocarted here.  do_mpx_bounds() can error out if
it sees an invalid bndregno.  We still send the signal with the 
whether or not we filled the 'info' in do_mpx_bounds().

Can't this leak some kernel stack out in the 'info'?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-30 Thread Dave Hansen 
 +void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
 + struct xsave_struct *xsave_buf)
 +{
 + struct mpx_insn insn;
 + uint8_t bndregno;
 + unsigned long addr_vio;
 +
 + addr_vio = mpx_insn_decode(insn, regs);
 +
 + bndregno = X86_MODRM_REG(insn.modrm.value);
 + if (bndregno  3)
 + return;
 +
 + /* Note: the upper 32 bits are ignored in 32-bit mode. */
 + info-si_lower = (void __user *)(unsigned long)
 + (xsave_buf-bndregs.bndregs[2*bndregno]);
 + info-si_upper = (void __user *)(unsigned long)
 + (~xsave_buf-bndregs.bndregs[2*bndregno+1]);
 + info-si_addr_lsb = 0;
 + info-si_signo = SIGSEGV;
 + info-si_errno = 0;
 + info-si_code = SEGV_BNDERR;
 + info-si_addr = (void __user *)addr_vio;
 +}
 diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
 index 611b6ec..b2a916b 100644
 --- a/arch/x86/kernel/traps.c
 +++ b/arch/x86/kernel/traps.c
 @@ -284,6 +284,7 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
 error_code)
   unsigned long status;
   struct xsave_struct *xsave_buf;
   struct task_struct *tsk = current;
 + siginfo_t info;
  
   prev_state = exception_enter();
   if (notify_die(DIE_TRAP, bounds, regs, error_code,
 @@ -316,6 +317,11 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
 error_code)
   break;
  
   case 1: /* Bound violation. */
 + do_mpx_bounds(regs, info, xsave_buf);
 + do_trap(X86_TRAP_BR, SIGSEGV, bounds, regs,
 + error_code, info);
 + break;
 +
   case 0: /* No exception caused by Intel MPX operations. */
   do_trap(X86_TRAP_BR, SIGSEGV, bounds, regs, error_code, NULL);
   break;
 

So, siginfo is stack-allocarted here.  do_mpx_bounds() can error out if
it sees an invalid bndregno.  We still send the signal with the info
whether or not we filled the 'info' in do_mpx_bounds().

Can't this leak some kernel stack out in the 'info'?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-30 Thread Ren Qiaowei 

On 10/31/2014 06:38 AM, Dave Hansen wrote:

+void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
+   struct xsave_struct *xsave_buf)
+{
+   struct mpx_insn insn;
+   uint8_t bndregno;
+   unsigned long addr_vio;
+
+   addr_vio = mpx_insn_decode(insn, regs);
+
+   bndregno = X86_MODRM_REG(insn.modrm.value);
+   if (bndregno  3)
+   return;
+
+   /* Note: the upper 32 bits are ignored in 32-bit mode. */
+   info-si_lower = (void __user *)(unsigned long)
+   (xsave_buf-bndregs.bndregs[2*bndregno]);
+   info-si_upper = (void __user *)(unsigned long)
+   (~xsave_buf-bndregs.bndregs[2*bndregno+1]);
+   info-si_addr_lsb = 0;
+   info-si_signo = SIGSEGV;
+   info-si_errno = 0;
+   info-si_code = SEGV_BNDERR;
+   info-si_addr = (void __user *)addr_vio;
+}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 611b6ec..b2a916b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -284,6 +284,7 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
error_code)
unsigned long status;
struct xsave_struct *xsave_buf;
struct task_struct *tsk = current;
+   siginfo_t info;

prev_state = exception_enter();
if (notify_die(DIE_TRAP, bounds, regs, error_code,
@@ -316,6 +317,11 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long 
error_code)
break;

case 1: /* Bound violation. */
+   do_mpx_bounds(regs, info, xsave_buf);
+   do_trap(X86_TRAP_BR, SIGSEGV, bounds, regs,
+   error_code, info);
+   break;
+
case 0: /* No exception caused by Intel MPX operations. */
do_trap(X86_TRAP_BR, SIGSEGV, bounds, regs, error_code, NULL);
break;



So, siginfo is stack-allocarted here.  do_mpx_bounds() can error out if
it sees an invalid bndregno.  We still send the signal with the info
whether or not we filled the 'info' in do_mpx_bounds().

Can't this leak some kernel stack out in the 'info'?



This should check the return value of do_mpx_bounds and should be fixed.

Thanks,
Qiaowei

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-28 Thread Ren Qiaowei 

On 10/28/2014 04:36 AM, Thomas Gleixner wrote:

On Mon, 27 Oct 2014, Ren, Qiaowei wrote:

On 2014-10-24, Thomas Gleixner wrote:

On Sun, 12 Oct 2014, Qiaowei Ren wrote:


This patch sets bound violation fields of siginfo struct in #BR
exception handler by decoding the user instruction and constructing
the faulting pointer.

This patch does't use the generic decoder, and implements a limited
special-purpose decoder to decode MPX instructions, simply because
the generic decoder is very heavyweight not just in terms of
performance but in terms of interface -- because it has to.


My question still stands why using the existing decoder is an issue.
Performance is a complete non issue in case of a bounds violation and
the interface argument is just silly, really.



As hpa said, we only need to decode several mpx instructions
including BNDCL/BNDCU, and general decoder looks like a little
heavy. Peter, what do you think about it?


You're repeating yourself. Care to read the discussion about this from
the last round of review again?



Ok. I will go through it again. Thanks.

- Qiaowei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-28 Thread Ren Qiaowei 

On 10/28/2014 04:36 AM, Thomas Gleixner wrote:

On Mon, 27 Oct 2014, Ren, Qiaowei wrote:

On 2014-10-24, Thomas Gleixner wrote:

On Sun, 12 Oct 2014, Qiaowei Ren wrote:


This patch sets bound violation fields of siginfo struct in #BR
exception handler by decoding the user instruction and constructing
the faulting pointer.

This patch does't use the generic decoder, and implements a limited
special-purpose decoder to decode MPX instructions, simply because
the generic decoder is very heavyweight not just in terms of
performance but in terms of interface -- because it has to.


My question still stands why using the existing decoder is an issue.
Performance is a complete non issue in case of a bounds violation and
the interface argument is just silly, really.



As hpa said, we only need to decode several mpx instructions
including BNDCL/BNDCU, and general decoder looks like a little
heavy. Peter, what do you think about it?


You're repeating yourself. Care to read the discussion about this from
the last round of review again?



Ok. I will go through it again. Thanks.

- Qiaowei
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-27 Thread Thomas Gleixner 
On Mon, 27 Oct 2014, Ren, Qiaowei wrote:
> On 2014-10-24, Thomas Gleixner wrote:
> > On Sun, 12 Oct 2014, Qiaowei Ren wrote:
> > 
> >> This patch sets bound violation fields of siginfo struct in #BR
> >> exception handler by decoding the user instruction and constructing
> >> the faulting pointer.
> >> 
> >> This patch does't use the generic decoder, and implements a limited
> >> special-purpose decoder to decode MPX instructions, simply because
> >> the generic decoder is very heavyweight not just in terms of
> >> performance but in terms of interface -- because it has to.
> > 
> > My question still stands why using the existing decoder is an issue.
> > Performance is a complete non issue in case of a bounds violation and
> > the interface argument is just silly, really.
> > 
> 
> As hpa said, we only need to decode several mpx instructions
> including BNDCL/BNDCU, and general decoder looks like a little
> heavy. Peter, what do you think about it?

You're repeating yourself. Care to read the discussion about this from
the last round of review again?

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-27 Thread Thomas Gleixner 
On Mon, 27 Oct 2014, Ren, Qiaowei wrote:
 On 2014-10-24, Thomas Gleixner wrote:
  On Sun, 12 Oct 2014, Qiaowei Ren wrote:
  
  This patch sets bound violation fields of siginfo struct in #BR
  exception handler by decoding the user instruction and constructing
  the faulting pointer.
  
  This patch does't use the generic decoder, and implements a limited
  special-purpose decoder to decode MPX instructions, simply because
  the generic decoder is very heavyweight not just in terms of
  performance but in terms of interface -- because it has to.
  
  My question still stands why using the existing decoder is an issue.
  Performance is a complete non issue in case of a bounds violation and
  the interface argument is just silly, really.
  
 
 As hpa said, we only need to decode several mpx instructions
 including BNDCL/BNDCU, and general decoder looks like a little
 heavy. Peter, what do you think about it?

You're repeating yourself. Care to read the discussion about this from
the last round of review again?

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-26 Thread Ren, Qiaowei 


On 2014-10-24, Thomas Gleixner wrote:
> On Sun, 12 Oct 2014, Qiaowei Ren wrote:
> 
>> This patch sets bound violation fields of siginfo struct in #BR
>> exception handler by decoding the user instruction and constructing
>> the faulting pointer.
>> 
>> This patch does't use the generic decoder, and implements a limited
>> special-purpose decoder to decode MPX instructions, simply because
>> the generic decoder is very heavyweight not just in terms of
>> performance but in terms of interface -- because it has to.
> 
> My question still stands why using the existing decoder is an issue.
> Performance is a complete non issue in case of a bounds violation and
> the interface argument is just silly, really.
> 

As hpa said, we only need to decode several mpx instructions including 
BNDCL/BNDCU, and general decoder looks like a little heavy. Peter, what do you 
think about it?

Thanks,
Qiaowei

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-26 Thread Ren, Qiaowei 


On 2014-10-24, Thomas Gleixner wrote:
 On Sun, 12 Oct 2014, Qiaowei Ren wrote:
 
 This patch sets bound violation fields of siginfo struct in #BR
 exception handler by decoding the user instruction and constructing
 the faulting pointer.
 
 This patch does't use the generic decoder, and implements a limited
 special-purpose decoder to decode MPX instructions, simply because
 the generic decoder is very heavyweight not just in terms of
 performance but in terms of interface -- because it has to.
 
 My question still stands why using the existing decoder is an issue.
 Performance is a complete non issue in case of a bounds violation and
 the interface argument is just silly, really.
 

As hpa said, we only need to decode several mpx instructions including 
BNDCL/BNDCU, and general decoder looks like a little heavy. Peter, what do you 
think about it?

Thanks,
Qiaowei

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-24 Thread Thomas Gleixner 
On Sun, 12 Oct 2014, Qiaowei Ren wrote:

> This patch sets bound violation fields of siginfo struct in #BR
> exception handler by decoding the user instruction and constructing
> the faulting pointer.
> 
> This patch does't use the generic decoder, and implements a limited
> special-purpose decoder to decode MPX instructions, simply because the
> generic decoder is very heavyweight not just in terms of performance
> but in terms of interface -- because it has to.

My question still stands why using the existing decoder is an
issue. Performance is a complete non issue in case of a bounds
violation and the interface argument is just silly, really.
 
Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-24 Thread Thomas Gleixner 
On Sun, 12 Oct 2014, Qiaowei Ren wrote:

 This patch sets bound violation fields of siginfo struct in #BR
 exception handler by decoding the user instruction and constructing
 the faulting pointer.
 
 This patch does't use the generic decoder, and implements a limited
 special-purpose decoder to decode MPX instructions, simply because the
 generic decoder is very heavyweight not just in terms of performance
 but in terms of interface -- because it has to.

My question still stands why using the existing decoder is an
issue. Performance is a complete non issue in case of a bounds
violation and the interface argument is just silly, really.
 
Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-11 Thread Qiaowei Ren 
This patch sets bound violation fields of siginfo struct in #BR
exception handler by decoding the user instruction and constructing
the faulting pointer.

This patch does't use the generic decoder, and implements a limited
special-purpose decoder to decode MPX instructions, simply because the
generic decoder is very heavyweight not just in terms of performance
but in terms of interface -- because it has to.

Signed-off-by: Qiaowei Ren 
---
 arch/x86/include/asm/mpx.h |   23 
 arch/x86/kernel/mpx.c  |  299 
 arch/x86/kernel/traps.c|6 +
 3 files changed, 328 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mpx.h b/arch/x86/include/asm/mpx.h
index b7598ac..780af63 100644
--- a/arch/x86/include/asm/mpx.h
+++ b/arch/x86/include/asm/mpx.h
@@ -3,6 +3,7 @@
 
 #include 
 #include 
+#include 
 
 #ifdef CONFIG_X86_64
 
@@ -44,15 +45,37 @@
 #define MPX_BNDSTA_ERROR_CODE  0x3
 #define MPX_BD_ENTRY_VALID_FLAG0x1
 
+struct mpx_insn {
+   struct insn_field rex_prefix;   /* REX prefix */
+   struct insn_field modrm;
+   struct insn_field sib;
+   struct insn_field displacement;
+
+   unsigned char addr_bytes;   /* effective address size */
+   unsigned char limit;
+   unsigned char x86_64;
+
+   const unsigned char *kaddr; /* kernel address of insn to analyze */
+   const unsigned char *next_byte;
+};
+
+#define MAX_MPX_INSN_SIZE  15
+
 unsigned long mpx_mmap(unsigned long len);
 
 #ifdef CONFIG_X86_INTEL_MPX
 int do_mpx_bt_fault(struct xsave_struct *xsave_buf);
+void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
+   struct xsave_struct *xsave_buf);
 #else
 static inline int do_mpx_bt_fault(struct xsave_struct *xsave_buf)
 {
return -EINVAL;
 }
+static inline void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
+   struct xsave_struct *xsave_buf)
+{
+}
 #endif /* CONFIG_X86_INTEL_MPX */
 
 #endif /* _ASM_X86_MPX_H */
diff --git a/arch/x86/kernel/mpx.c b/arch/x86/kernel/mpx.c
index 2103b5e..b7e4c0e 100644
--- a/arch/x86/kernel/mpx.c
+++ b/arch/x86/kernel/mpx.c
@@ -10,6 +10,275 @@
 #include 
 #include 
 
+enum reg_type {
+   REG_TYPE_RM = 0,
+   REG_TYPE_INDEX,
+   REG_TYPE_BASE,
+};
+
+static unsigned long get_reg(struct mpx_insn *insn, struct pt_regs *regs,
+enum reg_type type)
+{
+   int regno = 0;
+   unsigned char modrm = (unsigned char)insn->modrm.value;
+   unsigned char sib = (unsigned char)insn->sib.value;
+
+   static const int regoff[] = {
+   offsetof(struct pt_regs, ax),
+   offsetof(struct pt_regs, cx),
+   offsetof(struct pt_regs, dx),
+   offsetof(struct pt_regs, bx),
+   offsetof(struct pt_regs, sp),
+   offsetof(struct pt_regs, bp),
+   offsetof(struct pt_regs, si),
+   offsetof(struct pt_regs, di),
+#ifdef CONFIG_X86_64
+   offsetof(struct pt_regs, r8),
+   offsetof(struct pt_regs, r9),
+   offsetof(struct pt_regs, r10),
+   offsetof(struct pt_regs, r11),
+   offsetof(struct pt_regs, r12),
+   offsetof(struct pt_regs, r13),
+   offsetof(struct pt_regs, r14),
+   offsetof(struct pt_regs, r15),
+#endif
+   };
+
+   switch (type) {
+   case REG_TYPE_RM:
+   regno = X86_MODRM_RM(modrm);
+   if (X86_REX_B(insn->rex_prefix.value) == 1)
+   regno += 8;
+   break;
+
+   case REG_TYPE_INDEX:
+   regno = X86_SIB_INDEX(sib);
+   if (X86_REX_X(insn->rex_prefix.value) == 1)
+   regno += 8;
+   break;
+
+   case REG_TYPE_BASE:
+   regno = X86_SIB_BASE(sib);
+   if (X86_REX_B(insn->rex_prefix.value) == 1)
+   regno += 8;
+   break;
+
+   default:
+   break;
+   }
+
+   return regs_get_register(regs, regoff[regno]);
+}
+
+/*
+ * return the address being referenced be instruction
+ * for rm=3 returning the content of the rm reg
+ * for rm!=3 calculates the address using SIB and Disp
+ */
+static unsigned long get_addr_ref(struct mpx_insn *insn, struct pt_regs *regs)
+{
+   unsigned long addr;
+   unsigned long base;
+   unsigned long indx;
+   unsigned char modrm = (unsigned char)insn->modrm.value;
+   unsigned char sib = (unsigned char)insn->sib.value;
+
+   if (X86_MODRM_MOD(modrm) == 3) {
+   addr = get_reg(insn, regs, REG_TYPE_RM);
+   } else {
+   if (insn->sib.nbytes) {
+   base = get_reg(insn, regs, REG_TYPE_BASE);
+   indx = get_reg(insn, regs, REG_TYPE_INDEX);
+   addr = base + indx * (1 << X86_SIB_SCALE(sib));
+   } else {
+   addr = get_reg(insn,

[PATCH v9 09/12] x86, mpx: decode MPX instruction to get bound violation information

2014-10-11 Thread Qiaowei Ren 
This patch sets bound violation fields of siginfo struct in #BR
exception handler by decoding the user instruction and constructing
the faulting pointer.

This patch does't use the generic decoder, and implements a limited
special-purpose decoder to decode MPX instructions, simply because the
generic decoder is very heavyweight not just in terms of performance
but in terms of interface -- because it has to.

Signed-off-by: Qiaowei Ren qiaowei@intel.com
---
 arch/x86/include/asm/mpx.h |   23 
 arch/x86/kernel/mpx.c  |  299 
 arch/x86/kernel/traps.c|6 +
 3 files changed, 328 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/mpx.h b/arch/x86/include/asm/mpx.h
index b7598ac..780af63 100644
--- a/arch/x86/include/asm/mpx.h
+++ b/arch/x86/include/asm/mpx.h
@@ -3,6 +3,7 @@
 
 #include linux/types.h
 #include asm/ptrace.h
+#include asm/insn.h
 
 #ifdef CONFIG_X86_64
 
@@ -44,15 +45,37 @@
 #define MPX_BNDSTA_ERROR_CODE  0x3
 #define MPX_BD_ENTRY_VALID_FLAG0x1
 
+struct mpx_insn {
+   struct insn_field rex_prefix;   /* REX prefix */
+   struct insn_field modrm;
+   struct insn_field sib;
+   struct insn_field displacement;
+
+   unsigned char addr_bytes;   /* effective address size */
+   unsigned char limit;
+   unsigned char x86_64;
+
+   const unsigned char *kaddr; /* kernel address of insn to analyze */
+   const unsigned char *next_byte;
+};
+
+#define MAX_MPX_INSN_SIZE  15
+
 unsigned long mpx_mmap(unsigned long len);
 
 #ifdef CONFIG_X86_INTEL_MPX
 int do_mpx_bt_fault(struct xsave_struct *xsave_buf);
+void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
+   struct xsave_struct *xsave_buf);
 #else
 static inline int do_mpx_bt_fault(struct xsave_struct *xsave_buf)
 {
return -EINVAL;
 }
+static inline void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
+   struct xsave_struct *xsave_buf)
+{
+}
 #endif /* CONFIG_X86_INTEL_MPX */
 
 #endif /* _ASM_X86_MPX_H */
diff --git a/arch/x86/kernel/mpx.c b/arch/x86/kernel/mpx.c
index 2103b5e..b7e4c0e 100644
--- a/arch/x86/kernel/mpx.c
+++ b/arch/x86/kernel/mpx.c
@@ -10,6 +10,275 @@
 #include linux/syscalls.h
 #include asm/mpx.h
 
+enum reg_type {
+   REG_TYPE_RM = 0,
+   REG_TYPE_INDEX,
+   REG_TYPE_BASE,
+};
+
+static unsigned long get_reg(struct mpx_insn *insn, struct pt_regs *regs,
+enum reg_type type)
+{
+   int regno = 0;
+   unsigned char modrm = (unsigned char)insn-modrm.value;
+   unsigned char sib = (unsigned char)insn-sib.value;
+
+   static const int regoff[] = {
+   offsetof(struct pt_regs, ax),
+   offsetof(struct pt_regs, cx),
+   offsetof(struct pt_regs, dx),
+   offsetof(struct pt_regs, bx),
+   offsetof(struct pt_regs, sp),
+   offsetof(struct pt_regs, bp),
+   offsetof(struct pt_regs, si),
+   offsetof(struct pt_regs, di),
+#ifdef CONFIG_X86_64
+   offsetof(struct pt_regs, r8),
+   offsetof(struct pt_regs, r9),
+   offsetof(struct pt_regs, r10),
+   offsetof(struct pt_regs, r11),
+   offsetof(struct pt_regs, r12),
+   offsetof(struct pt_regs, r13),
+   offsetof(struct pt_regs, r14),
+   offsetof(struct pt_regs, r15),
+#endif
+   };
+
+   switch (type) {
+   case REG_TYPE_RM:
+   regno = X86_MODRM_RM(modrm);
+   if (X86_REX_B(insn-rex_prefix.value) == 1)
+   regno += 8;
+   break;
+
+   case REG_TYPE_INDEX:
+   regno = X86_SIB_INDEX(sib);
+   if (X86_REX_X(insn-rex_prefix.value) == 1)
+   regno += 8;
+   break;
+
+   case REG_TYPE_BASE:
+   regno = X86_SIB_BASE(sib);
+   if (X86_REX_B(insn-rex_prefix.value) == 1)
+   regno += 8;
+   break;
+
+   default:
+   break;
+   }
+
+   return regs_get_register(regs, regoff[regno]);
+}
+
+/*
+ * return the address being referenced be instruction
+ * for rm=3 returning the content of the rm reg
+ * for rm!=3 calculates the address using SIB and Disp
+ */
+static unsigned long get_addr_ref(struct mpx_insn *insn, struct pt_regs *regs)
+{
+   unsigned long addr;
+   unsigned long base;
+   unsigned long indx;
+   unsigned char modrm = (unsigned char)insn-modrm.value;
+   unsigned char sib = (unsigned char)insn-sib.value;
+
+   if (X86_MODRM_MOD(modrm) == 3) {
+   addr = get_reg(insn, regs, REG_TYPE_RM);
+   } else {
+   if (insn-sib.nbytes) {
+   base = get_reg(insn, regs, REG_TYPE_BASE);
+   indx = get_reg(insn, regs, REG_TYPE_INDEX);
+   addr = base + indx * (1  X86_SIB_SCALE(sib));