RE: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 2014-06-20, H. Peter Anvin wrote: > On 06/19/2014 10:04 AM, Dave Hansen wrote: >> >> Could you please support this position with some data? I'm a bit >> skeptical that instruction decoding is going to be a >> performance-critical path. >> >> I also don't see the extra field that you talked about in the >> previous thread? What's the extra field? I see a 'limit' vs. >> 'length', but you don't use 'length' at all, so I think you can use >> it instead, or at least union it. >> >> I've taken a quick stab at trying to consolidate things. I think I >> may have screwed up this: >> >> insn->limit = MAX_MPX_INSN_SIZE - bytes; >> >> Qiaowei, is there anything fundamentally broken with what I've got here? >> > Firstly instructions will be got from user pointer stored in 'ip', and then validate_next() will use 'limit' to make sure that next sizeof(t) bytes can be on the same instruction. As hpa said, generic decoder, including struct insn and implementation of decoding, is very heavyweight because it has to. So MPX specific decoding should be better choice. > So I encouraged Qiaowei to do a limited special-purpose decoder, > simply because the glue to use the generic decoder was almost as > large. I am overall not a huge fan of using the generic decoder in > constrained situation, because the generic decoder is very heavyweight > not just in terms of performance but in terms of interface -- because it has > to. > Thanks, Qiaowei
Re: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 06/19/2014 10:04 AM, Dave Hansen wrote: > > Could you please support this position with some data? I'm a bit > skeptical that instruction decoding is going to be a > performance-critical path. > > I also don't see the extra field that you talked about in the previous > thread? What's the extra field? I see a 'limit' vs. 'length', but you > don't use 'length' at all, so I think you can use it instead, or at > least union it. > > I've taken a quick stab at trying to consolidate things. I think I may > have screwed up this: > > insn->limit = MAX_MPX_INSN_SIZE - bytes; > > Qiaowei, is there anything fundamentally broken with what I've got here? > So I encouraged Qiaowei to do a limited special-purpose decoder, simply because the glue to use the generic decoder was almost as large. I am overall not a huge fan of using the generic decoder in constrained situation, because the generic decoder is very heavyweight not just in terms of performance but in terms of interface -- because it has to. -hpa -- 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 v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 06/18/2014 11:53 PM, Ren, Qiaowei wrote:
> On 2014-06-19, Borislav Petkov wrote:
>> On Thu, Jun 19, 2014 at 01:13:48AM +, Ren, Qiaowei wrote:
>>> On 2014-06-18, Borislav Petkov wrote:
On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote:
This whole insn decoding machinery above looks like adapted from
arch/x86/lib/insn.c. You should merge it with the generic code in
insn.c instead of homegrowing it here only for the purposes of MPX.
And if it doesn't work for your needs, you should should extend
the generic code to do so.
>>
>>> Petkov, as we discussed on initial version of this patchset, general
>>> insn framework didn't work out well and I have tried to use generic
>>> struct insn, insn_field, etc. for obvious benefits.
>>
>> Let me repeat myself: "And if it doesn't work for your needs, you
>> should extend the generic code to do so."
>>
>> We don't do homegrown almost-copies of generic code.
>>
> I see. If possible, I will be very happy to use or extend generic
> code. But due to extra overhead caused by MPX, I have to use MPX
> specific decoding to do performance optimization.
Could you please support this position with some data? I'm a bit
skeptical that instruction decoding is going to be a
performance-critical path.
I also don't see the extra field that you talked about in the previous
thread? What's the extra field? I see a 'limit' vs. 'length', but you
don't use 'length' at all, so I think you can use it instead, or at
least union it.
I've taken a quick stab at trying to consolidate things. I think I may
have screwed up this:
insn->limit = MAX_MPX_INSN_SIZE - bytes;
Qiaowei, is there anything fundamentally broken with what I've got here?
---
b/arch/x86/include/asm/insn.h |1
b/arch/x86/include/asm/mpx.h | 14 ---
b/arch/x86/kernel/mpx.c | 183 ++
b/arch/x86/lib/insn.c | 43 +
4 files changed, 56 insertions(+), 185 deletions(-)
diff -puN arch/x86/kernel/mpx.c~consolidate-instruction-decoding arch/x86/kernel/mpx.c
--- a/arch/x86/kernel/mpx.c~consolidate-instruction-decoding 2014-06-19 09:53:53.894441788 -0700
+++ b/arch/x86/kernel/mpx.c 2014-06-19 09:53:53.909442460 -0700
@@ -3,6 +3,7 @@
#include
#include
#include
+#include
#include
/*
@@ -59,7 +60,7 @@ int mpx_unregister(struct task_struct *t
}
typedef enum {REG_TYPE_RM, REG_TYPE_INDEX, REG_TYPE_BASE} reg_type_t;
-static unsigned long get_reg(struct mpx_insn *insn, struct pt_regs *regs,
+static unsigned long get_reg(struct insn *insn, struct pt_regs *regs,
reg_type_t type)
{
int regno = 0;
@@ -118,7 +119,7 @@ static unsigned long get_reg(struct mpx_
* 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)
+static unsigned long get_addr_ref(struct insn *insn, struct pt_regs *regs)
{
unsigned long addr;
unsigned long base;
@@ -142,182 +143,22 @@ static unsigned long get_addr_ref(struct
return addr;
}
-/* Verify next sizeof(t) bytes can be on the same instruction */
-#define validate_next(t, insn, n) \
- ((insn)->next_byte + sizeof(t) + n - (insn)->kaddr <= (insn)->limit)
-
-#define __get_next(t, insn) \
-({ \
- t r = *(t *)insn->next_byte; \
- insn->next_byte += sizeof(t); \
- r;\
-})
-
-#define __peek_next(t, insn) \
-({ \
- t r = *(t *)insn->next_byte; \
- r;\
-})
-
-#define get_next(t, insn) \
-({ \
- if (unlikely(!validate_next(t, insn, 0))) \
- goto err_out; \
- __get_next(t, insn); \
-})
-
-#define peek_next(t, insn) \
-({ \
- if (unlikely(!validate_next(t, insn, 0))) \
- goto err_out; \
- __peek_next(t, insn); \
-})
-
-static void mpx_insn_get_prefixes(struct mpx_insn *insn)
-{
- unsigned char b;
-
- /* Decode legacy prefix and REX prefix */
- b = peek_next(unsigned char, insn);
- while (b != 0x0f) {
- /*
- * look for a rex prefix
- * a REX prefix cannot be followed by a legacy prefix.
- */
- if (insn->x86_64 && ((b&0xf0) == 0x40)) {
- insn->rex_prefix.value = b;
- insn->rex_prefix.nbytes = 1;
- insn->next_byte++;
- break;
- }
-
- /* check the other legacy prefixes */
- switch (b) {
- case 0xf2:
- case 0xf3:
- case 0xf0:
- case 0x64:
- case 0x65:
- case 0x2e:
- case 0x3e:
- case 0x26:
- case 0x36:
- case 0x66:
- case 0x67:
- insn->next_byte++;
- break;
- default: /* everything else is garbage */
- goto err_out;
- }
- b = peek_next(unsigned char, insn);
- }
-
-err_out:
- return;
-}
-
-static void mpx_insn_get_modrm(struct mpx_insn *insn)
-{
- insn->modrm.value = get_next(unsigned char, insn);
- insn->modrm.nbytes = 1;
-
-err_out:
- return;
-}
-
-static void mpx_insn_get_sib(struct mpx_insn *insn)
-{
- unsigned char modrm = (unsigned char)insn->modrm.value;
-
- if (X86_MODRM_MOD(modrm) != 3 && X86_MODRM_RM(modrm) == 4) {
- insn->sib.value = get_next(unsigned
RE: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 2014-06-19, Borislav Petkov wrote:
> On Thu, Jun 19, 2014 at 01:13:48AM +, Ren, Qiaowei wrote:
>> On 2014-06-18, Borislav Petkov wrote:
>>> On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote:
>>>
>>> This whole insn decoding machinery above looks like adapted from
>>> arch/x86/lib/insn.c. You should merge it with the generic code in
>>> insn.c instead of homegrowing it here only for the purposes of MPX.
>>> And if it doesn't work for your needs, you should should extend
>>> the generic code to do so.
>
>> Petkov, as we discussed on initial version of this patchset, general
>> insn framework didn't work out well and I have tried to use generic
>> struct insn, insn_field, etc. for obvious benefits.
>
> Let me repeat myself: "And if it doesn't work for your needs, you
> should extend the generic code to do so."
>
> We don't do homegrown almost-copies of generic code.
>
I see. If possible, I will be very happy to use or extend generic code. But due
to extra overhead caused by MPX, I have to use MPX specific decoding to do
performance optimization.
You can check the discussion on this: https://lkml.org/lkml/2014/1/11/190
Thanks,
Qiaowei
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On Thu, Jun 19, 2014 at 01:13:48AM +, Ren, Qiaowei wrote: > On 2014-06-18, Borislav Petkov wrote: > > On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote: > > > > This whole insn decoding machinery above looks like adapted from > > arch/x86/lib/insn.c. You should merge it with the generic code in > > insn.c instead of homegrowing it here only for the purposes of MPX. > > And if it doesn't work for your needs, you should should extend the > > generic code to do so. > Petkov, as we discussed on initial version of this patchset, general > insn framework didn't work out well and I have tried to use generic > struct insn, insn_field, etc. for obvious benefits. Let me repeat myself: "And if it doesn't work for your needs, you should extend the generic code to do so." We don't do homegrown almost-copies of generic code. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- 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 v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On Thu, Jun 19, 2014 at 01:13:48AM +, Ren, Qiaowei wrote: On 2014-06-18, Borislav Petkov wrote: On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote: This whole insn decoding machinery above looks like adapted from arch/x86/lib/insn.c. You should merge it with the generic code in insn.c instead of homegrowing it here only for the purposes of MPX. And if it doesn't work for your needs, you should should extend the generic code to do so. Petkov, as we discussed on initial version of this patchset, general insn framework didn't work out well and I have tried to use generic struct insn, insn_field, etc. for obvious benefits. Let me repeat myself: And if it doesn't work for your needs, you should extend the generic code to do so. We don't do homegrown almost-copies of generic code. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- 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 v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 2014-06-19, Borislav Petkov wrote:
On Thu, Jun 19, 2014 at 01:13:48AM +, Ren, Qiaowei wrote:
On 2014-06-18, Borislav Petkov wrote:
On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote:
This whole insn decoding machinery above looks like adapted from
arch/x86/lib/insn.c. You should merge it with the generic code in
insn.c instead of homegrowing it here only for the purposes of MPX.
And if it doesn't work for your needs, you should should extend
the generic code to do so.
Petkov, as we discussed on initial version of this patchset, general
insn framework didn't work out well and I have tried to use generic
struct insn, insn_field, etc. for obvious benefits.
Let me repeat myself: And if it doesn't work for your needs, you
should extend the generic code to do so.
We don't do homegrown almost-copies of generic code.
I see. If possible, I will be very happy to use or extend generic code. But due
to extra overhead caused by MPX, I have to use MPX specific decoding to do
performance optimization.
You can check the discussion on this: https://lkml.org/lkml/2014/1/11/190
Thanks,
Qiaowei
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 06/18/2014 11:53 PM, Ren, Qiaowei wrote:
On 2014-06-19, Borislav Petkov wrote:
On Thu, Jun 19, 2014 at 01:13:48AM +, Ren, Qiaowei wrote:
On 2014-06-18, Borislav Petkov wrote:
On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote:
This whole insn decoding machinery above looks like adapted from
arch/x86/lib/insn.c. You should merge it with the generic code in
insn.c instead of homegrowing it here only for the purposes of MPX.
And if it doesn't work for your needs, you should should extend
the generic code to do so.
Petkov, as we discussed on initial version of this patchset, general
insn framework didn't work out well and I have tried to use generic
struct insn, insn_field, etc. for obvious benefits.
Let me repeat myself: And if it doesn't work for your needs, you
should extend the generic code to do so.
We don't do homegrown almost-copies of generic code.
I see. If possible, I will be very happy to use or extend generic
code. But due to extra overhead caused by MPX, I have to use MPX
specific decoding to do performance optimization.
Could you please support this position with some data? I'm a bit
skeptical that instruction decoding is going to be a
performance-critical path.
I also don't see the extra field that you talked about in the previous
thread? What's the extra field? I see a 'limit' vs. 'length', but you
don't use 'length' at all, so I think you can use it instead, or at
least union it.
I've taken a quick stab at trying to consolidate things. I think I may
have screwed up this:
insn-limit = MAX_MPX_INSN_SIZE - bytes;
Qiaowei, is there anything fundamentally broken with what I've got here?
---
b/arch/x86/include/asm/insn.h |1
b/arch/x86/include/asm/mpx.h | 14 ---
b/arch/x86/kernel/mpx.c | 183 ++
b/arch/x86/lib/insn.c | 43 +
4 files changed, 56 insertions(+), 185 deletions(-)
diff -puN arch/x86/kernel/mpx.c~consolidate-instruction-decoding arch/x86/kernel/mpx.c
--- a/arch/x86/kernel/mpx.c~consolidate-instruction-decoding 2014-06-19 09:53:53.894441788 -0700
+++ b/arch/x86/kernel/mpx.c 2014-06-19 09:53:53.909442460 -0700
@@ -3,6 +3,7 @@
#include linux/prctl.h
#include asm/mpx.h
#include asm/i387.h
+#include asm/insn.h
#include asm/fpu-internal.h
/*
@@ -59,7 +60,7 @@ int mpx_unregister(struct task_struct *t
}
typedef enum {REG_TYPE_RM, REG_TYPE_INDEX, REG_TYPE_BASE} reg_type_t;
-static unsigned long get_reg(struct mpx_insn *insn, struct pt_regs *regs,
+static unsigned long get_reg(struct insn *insn, struct pt_regs *regs,
reg_type_t type)
{
int regno = 0;
@@ -118,7 +119,7 @@ static unsigned long get_reg(struct mpx_
* 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)
+static unsigned long get_addr_ref(struct insn *insn, struct pt_regs *regs)
{
unsigned long addr;
unsigned long base;
@@ -142,182 +143,22 @@ static unsigned long get_addr_ref(struct
return addr;
}
-/* Verify next sizeof(t) bytes can be on the same instruction */
-#define validate_next(t, insn, n) \
- ((insn)-next_byte + sizeof(t) + n - (insn)-kaddr = (insn)-limit)
-
-#define __get_next(t, insn) \
-({ \
- t r = *(t *)insn-next_byte; \
- insn-next_byte += sizeof(t); \
- r;\
-})
-
-#define __peek_next(t, insn) \
-({ \
- t r = *(t *)insn-next_byte; \
- r;\
-})
-
-#define get_next(t, insn) \
-({ \
- if (unlikely(!validate_next(t, insn, 0))) \
- goto err_out; \
- __get_next(t, insn); \
-})
-
-#define peek_next(t, insn) \
-({ \
- if (unlikely(!validate_next(t, insn, 0))) \
- goto err_out; \
- __peek_next(t, insn); \
-})
-
-static void mpx_insn_get_prefixes(struct mpx_insn *insn)
-{
- unsigned char b;
-
- /* Decode legacy prefix and REX prefix */
- b = peek_next(unsigned char, insn);
- while (b != 0x0f) {
- /*
- * look for a rex prefix
- * a REX prefix cannot be followed by a legacy prefix.
- */
- if (insn-x86_64 ((b0xf0) == 0x40)) {
- insn-rex_prefix.value = b;
- insn-rex_prefix.nbytes = 1;
- insn-next_byte++;
- break;
- }
-
- /* check the other legacy prefixes */
- switch (b) {
- case 0xf2:
- case 0xf3:
- case 0xf0:
- case 0x64:
- case 0x65:
- case 0x2e:
- case 0x3e:
- case 0x26:
- case 0x36:
- case 0x66:
- case 0x67:
- insn-next_byte++;
- break;
- default: /* everything else is garbage */
- goto err_out;
- }
- b = peek_next(unsigned char, insn);
- }
-
-err_out:
- return;
-}
-
-static void mpx_insn_get_modrm(struct mpx_insn *insn)
-{
- insn-modrm.value = get_next(unsigned char, insn);
- insn-modrm.nbytes = 1;
-
-err_out:
- return;
-}
-
-static void mpx_insn_get_sib(struct mpx_insn *insn)
-{
- unsigned char modrm = (unsigned char)insn-modrm.value;
-
- if (X86_MODRM_MOD(modrm) != 3 X86_MODRM_RM(modrm) == 4) {
- insn-sib.value = get_next(unsigned char, insn);
-
Re: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 06/19/2014 10:04 AM, Dave Hansen wrote: Could you please support this position with some data? I'm a bit skeptical that instruction decoding is going to be a performance-critical path. I also don't see the extra field that you talked about in the previous thread? What's the extra field? I see a 'limit' vs. 'length', but you don't use 'length' at all, so I think you can use it instead, or at least union it. I've taken a quick stab at trying to consolidate things. I think I may have screwed up this: insn-limit = MAX_MPX_INSN_SIZE - bytes; Qiaowei, is there anything fundamentally broken with what I've got here? So I encouraged Qiaowei to do a limited special-purpose decoder, simply because the glue to use the generic decoder was almost as large. I am overall not a huge fan of using the generic decoder in constrained situation, because the generic decoder is very heavyweight not just in terms of performance but in terms of interface -- because it has to. -hpa -- 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 v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 2014-06-20, H. Peter Anvin wrote: On 06/19/2014 10:04 AM, Dave Hansen wrote: Could you please support this position with some data? I'm a bit skeptical that instruction decoding is going to be a performance-critical path. I also don't see the extra field that you talked about in the previous thread? What's the extra field? I see a 'limit' vs. 'length', but you don't use 'length' at all, so I think you can use it instead, or at least union it. I've taken a quick stab at trying to consolidate things. I think I may have screwed up this: insn-limit = MAX_MPX_INSN_SIZE - bytes; Qiaowei, is there anything fundamentally broken with what I've got here? Firstly instructions will be got from user pointer stored in 'ip', and then validate_next() will use 'limit' to make sure that next sizeof(t) bytes can be on the same instruction. As hpa said, generic decoder, including struct insn and implementation of decoding, is very heavyweight because it has to. So MPX specific decoding should be better choice. So I encouraged Qiaowei to do a limited special-purpose decoder, simply because the glue to use the generic decoder was almost as large. I am overall not a huge fan of using the generic decoder in constrained situation, because the generic decoder is very heavyweight not just in terms of performance but in terms of interface -- because it has to. Thanks, Qiaowei
RE: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 2014-06-18, Borislav Petkov wrote: > On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote: > > This whole insn decoding machinery above looks like adapted from > arch/x86/lib/insn.c. You should merge it with the generic code in > insn.c instead of homegrowing it here only for the purposes of MPX. > And if it doesn't work for your needs, you should should extend the > generic code to do so. I think we even talked about this last time. > > Also, make sure you run all your patches through checkpatch.pl before > submitting. > Petkov, as we discussed on initial version of this patchset, general insn framework didn't work out well and I have tried to use generic struct insn, insn_field, etc. for obvious benefits. Thanks, Qiaowei
Re: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On Wed, Jun 18, 2014 at 05:44:13PM +0800, 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.
>
> Signed-off-by: Qiaowei Ren
> ---
> arch/x86/include/asm/mpx.h | 23
> arch/x86/kernel/mpx.c | 294
>
> arch/x86/kernel/traps.c|6 +
> 3 files changed, 323 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_CODE0x3
> #define MPX_BD_ENTRY_VALID_FLAG 0x1
>
> +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_SIZE15
> +
> 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 4230c7b..650b282 100644
> --- a/arch/x86/kernel/mpx.c
> +++ b/arch/x86/kernel/mpx.c
> @@ -2,6 +2,270 @@
> #include
> #include
>
> +typedef enum {REG_TYPE_RM, REG_TYPE_INDEX, REG_TYPE_BASE} reg_type_t;
> +static unsigned long get_reg(struct mpx_insn *insn, struct pt_regs *regs,
> + reg_type_t 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, regs, REG_TYPE_RM);
> + }
> + addr +=
Re: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On Wed, Jun 18, 2014 at 05:44:13PM +0800, 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.
Signed-off-by: Qiaowei Ren qiaowei@intel.com
---
arch/x86/include/asm/mpx.h | 23
arch/x86/kernel/mpx.c | 294
arch/x86/kernel/traps.c|6 +
3 files changed, 323 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_CODE0x3
#define MPX_BD_ENTRY_VALID_FLAG 0x1
+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_SIZE15
+
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 4230c7b..650b282 100644
--- a/arch/x86/kernel/mpx.c
+++ b/arch/x86/kernel/mpx.c
@@ -2,6 +2,270 @@
#include linux/syscalls.h
#include asm/mpx.h
+typedef enum {REG_TYPE_RM, REG_TYPE_INDEX, REG_TYPE_BASE} reg_type_t;
+static unsigned long get_reg(struct mpx_insn *insn, struct pt_regs *regs,
+ reg_type_t 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, regs, REG_TYPE_RM);
+ }
+ addr += insn-displacement.value;
+ }
+
+ return addr;
+}
+
+/* Verify
RE: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information
On 2014-06-18, Borislav Petkov wrote: On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote: This whole insn decoding machinery above looks like adapted from arch/x86/lib/insn.c. You should merge it with the generic code in insn.c instead of homegrowing it here only for the purposes of MPX. And if it doesn't work for your needs, you should should extend the generic code to do so. I think we even talked about this last time. Also, make sure you run all your patches through checkpatch.pl before submitting. Petkov, as we discussed on initial version of this patchset, general insn framework didn't work out well and I have tried to use generic struct insn, insn_field, etc. for obvious benefits. Thanks, Qiaowei
