Re: [v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor

2017-05-15 Thread Borislav Petkov 
On Thu, May 11, 2017 at 07:13:57PM -0700, Ricardo Neri wrote:
> Sure I can. Would this trigger a v8 of my series? I was hoping v7 series
> could be merged and then start doing incremental work on top of it. Does
> it make sense?

I guess that's tip guys' call.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor

2017-05-11 Thread Ricardo Neri 
On Thu, 2017-05-04 at 13:02 +0200, Borislav Petkov wrote:
> On Wed, Apr 26, 2017 at 02:51:56PM -0700, Ricardo Neri wrote:
> > > > +seg >= 
> > > > current->active_mm->context.ldt->size)) {
> > > 
> > > ldt->size is the size of the descriptor table but you've shifted seg by
> > > 3. That selector index is shifted by 3 (to the left) to form an offset
> > > into the descriptor table because the entries there are 8 bytes.
> > 
> > I double-checked the ldt code and it seems to me that size refers to the
> > number of entries in the table; it is always multiplied by
> > LDT_ENTRY_SIZE [1], [2]. Am I missing something?
> 
> No, you're not. I fell into that wrongly named struct member trap.
> 
> So ldt_struct.size should actually be called ldt_struct.n_entries or
> similar. Because what's in there is now is not "size".
> 
> And then code like
> 
>   new_ldt->size * LDT_ENTRY_SIZE
> 
> would make much more sense if written like this:
> 
>   new_ldt->n_entries * LDT_ENTRY_SIZE
> 
> Would you fix that in a prepatch pls?
> 

Sure I can. Would this trigger a v8 of my series? I was hoping v7 series
could be merged and then start doing incremental work on top of it. Does
it make sense?

Thanks and BR,
Ricardo

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

Re: [v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor

2017-05-04 Thread Borislav Petkov 
On Wed, Apr 26, 2017 at 02:51:56PM -0700, Ricardo Neri wrote:
> > > +  seg >= current->active_mm->context.ldt->size)) {
> > 
> > ldt->size is the size of the descriptor table but you've shifted seg by
> > 3. That selector index is shifted by 3 (to the left) to form an offset
> > into the descriptor table because the entries there are 8 bytes.
> 
> I double-checked the ldt code and it seems to me that size refers to the
> number of entries in the table; it is always multiplied by
> LDT_ENTRY_SIZE [1], [2]. Am I missing something?

No, you're not. I fell into that wrongly named struct member trap.

So ldt_struct.size should actually be called ldt_struct.n_entries or
similar. Because what's in there is now is not "size".

And then code like

new_ldt->size * LDT_ENTRY_SIZE

would make much more sense if written like this:

new_ldt->n_entries * LDT_ENTRY_SIZE

Would you fix that in a prepatch pls?

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor

2017-04-26 Thread Ricardo Neri 
On Wed, 2017-04-19 at 12:26 +0200, Borislav Petkov wrote:
> On Tue, Mar 07, 2017 at 04:32:40PM -0800, Ricardo Neri wrote:
> > The segment descriptor contains information that is relevant to how linear
> > address need to be computed. It contains the default size of addresses as
> > well as the base address of the segment. Thus, given a segment selector,
> > we ought look at segment descriptor to correctly calculate the linear
> > address.
> > 
> > In protected mode, the segment selector might indicate a segment
> > descriptor from either the global descriptor table or a local descriptor
> > table. Both cases are considered in this function.
> > 
> > This function is the initial implementation for subsequent functions that
> > will obtain the aforementioned attributes of the segment descriptor.
> > 
> > Cc: Dave Hansen 
> > Cc: Adam Buchbinder 
> > Cc: Colin Ian King 
> > Cc: Lorenzo Stoakes 
> > Cc: Qiaowei Ren 
> > Cc: Arnaldo Carvalho de Melo 
> > Cc: Masami Hiramatsu 
> > Cc: Adrian Hunter 
> > Cc: Kees Cook 
> > Cc: Thomas Garnier 
> > Cc: Peter Zijlstra 
> > Cc: Borislav Petkov 
> > Cc: Dmitry Vyukov 
> > Cc: Ravi V. Shankar 
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/lib/insn-eval.c | 61 
> > 
> >  1 file changed, 61 insertions(+)
> > 
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index 8d45df8..8608adf 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -5,9 +5,13 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #include 
> >  
> >  enum reg_type {
> > @@ -294,6 +298,63 @@ static int get_reg_offset(struct insn *insn, struct 
> > pt_regs *regs,
> >  }
> >  
> >  /**
> > + * get_desc() - Obtain address of segment descriptor
> > + * @seg:   Segment selector
> 
> Maybe that should be
> 
> @sel
> 
> if it is a sel-ector. :)

It makes sense. I will rename it.
> 
> And using "sel" makes more sense then when you look at:
> 
>   desc_base = sel & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);
> 
> for example:
> 
> > + * @desc:  Pointer to the selected segment descriptor
> > + *
> > + * Given a segment selector, obtain a memory pointer to the segment
> 
> s/memory //

Will update it.
> 
> > + * descriptor. Both global and local descriptor tables are supported.
> > + * desc will contain the address of the descriptor.
> > + *
> > + * Return: 0 if success, -EINVAL if failure
> 
> Why isn't this function returning the pointer or NULL on error? Maybe
> the later patches have an answer and I'll discover it if I continue
> reviewing :)

After revisiting the code, I don't see why the function cannot return
NULL.
> 
> > + */
> > +static int get_desc(unsigned short seg, struct desc_struct **desc)
> > +{
> > +   struct desc_ptr gdt_desc = {0, 0};
> > +   unsigned long desc_base;
> > +
> > +   if (!desc)
> > +   return -EINVAL;
> > +
> > +   desc_base = seg & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);
> 
> That looks useless as you're doing it below again.

Yes, it is useless. Please see my comment below.
> 
> > +
> > +#ifdef CONFIG_MODIFY_LDT_SYSCALL
> > +   if ((seg & SEGMENT_TI_MASK) == SEGMENT_LDT) {
> > +   seg >>= 3;
> > +
> > +   mutex_lock(>active_mm->context.lock);
> > +   if (unlikely(!current->active_mm->context.ldt ||
> 
> Is that really a fast path to complicate the if-test with an unlikely()?
> If not, you don't really need it.

I will remove it.
> 
> > +seg >= current->active_mm->context.ldt->size)) {
> 
> ldt->size is the size of the descriptor table but you've shifted seg by
> 3. That selector index is shifted by 3 (to the left) to form an offset
> into the descriptor table because the entries there are 8 bytes.

I double-checked the ldt code and it seems to me that size refers to the
number of entries in the table; it is always multiplied by
LDT_ENTRY_SIZE [1], [2]. Am I missing something?

> 
> So I *think* you wanna use the "useless" desc_base above... :)
> 
> > +   *desc = NULL;
> > +   mutex_unlock(>active_mm->context.lock);
> > +   return -EINVAL;
> > +   }
> > +
> > +   *desc = >active_mm->context.ldt->entries[seg];
> 
> ... and seg here as it is an index into the table.
> 
> > +   mutex_unlock(>active_mm->context.lock);
> > +   return 0;
> > +   }
> > +#endif
> > +   native_store_gdt(_desc);
> > +
> > +   /*
> > +* Bits [15:3] of the segment selector contain the index. Such
> >

Re: [v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor

2017-04-19 Thread Borislav Petkov 
On Tue, Mar 07, 2017 at 04:32:40PM -0800, Ricardo Neri wrote:
> The segment descriptor contains information that is relevant to how linear
> address need to be computed. It contains the default size of addresses as
> well as the base address of the segment. Thus, given a segment selector,
> we ought look at segment descriptor to correctly calculate the linear
> address.
> 
> In protected mode, the segment selector might indicate a segment
> descriptor from either the global descriptor table or a local descriptor
> table. Both cases are considered in this function.
> 
> This function is the initial implementation for subsequent functions that
> will obtain the aforementioned attributes of the segment descriptor.
> 
> Cc: Dave Hansen 
> Cc: Adam Buchbinder 
> Cc: Colin Ian King 
> Cc: Lorenzo Stoakes 
> Cc: Qiaowei Ren 
> Cc: Arnaldo Carvalho de Melo 
> Cc: Masami Hiramatsu 
> Cc: Adrian Hunter 
> Cc: Kees Cook 
> Cc: Thomas Garnier 
> Cc: Peter Zijlstra 
> Cc: Borislav Petkov 
> Cc: Dmitry Vyukov 
> Cc: Ravi V. Shankar 
> Cc: x...@kernel.org
> Signed-off-by: Ricardo Neri 
> ---
>  arch/x86/lib/insn-eval.c | 61 
> 
>  1 file changed, 61 insertions(+)
> 
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 8d45df8..8608adf 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -5,9 +5,13 @@
>   */
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  
>  enum reg_type {
> @@ -294,6 +298,63 @@ static int get_reg_offset(struct insn *insn, struct 
> pt_regs *regs,
>  }
>  
>  /**
> + * get_desc() - Obtain address of segment descriptor
> + * @seg: Segment selector

Maybe that should be

@sel

if it is a sel-ector. :)

And using "sel" makes more sense then when you look at:

desc_base = sel & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);

for example:

> + * @desc:Pointer to the selected segment descriptor
> + *
> + * Given a segment selector, obtain a memory pointer to the segment

s/memory //

> + * descriptor. Both global and local descriptor tables are supported.
> + * desc will contain the address of the descriptor.
> + *
> + * Return: 0 if success, -EINVAL if failure

Why isn't this function returning the pointer or NULL on error? Maybe
the later patches have an answer and I'll discover it if I continue
reviewing :)

> + */
> +static int get_desc(unsigned short seg, struct desc_struct **desc)
> +{
> + struct desc_ptr gdt_desc = {0, 0};
> + unsigned long desc_base;
> +
> + if (!desc)
> + return -EINVAL;
> +
> + desc_base = seg & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);

That looks useless as you're doing it below again.

> +
> +#ifdef CONFIG_MODIFY_LDT_SYSCALL
> + if ((seg & SEGMENT_TI_MASK) == SEGMENT_LDT) {
> + seg >>= 3;
> +
> + mutex_lock(>active_mm->context.lock);
> + if (unlikely(!current->active_mm->context.ldt ||

Is that really a fast path to complicate the if-test with an unlikely()?
If not, you don't really need it.

> +  seg >= current->active_mm->context.ldt->size)) {

ldt->size is the size of the descriptor table but you've shifted seg by
3. That selector index is shifted by 3 (to the left) to form an offset
into the descriptor table because the entries there are 8 bytes.

So I *think* you wanna use the "useless" desc_base above... :)

> + *desc = NULL;
> + mutex_unlock(>active_mm->context.lock);
> + return -EINVAL;
> + }
> +
> + *desc = >active_mm->context.ldt->entries[seg];

... and seg here as it is an index into the table.

> + mutex_unlock(>active_mm->context.lock);
> + return 0;
> + }
> +#endif
> + native_store_gdt(_desc);
> +
> + /*
> +  * Bits [15:3] of the segment selector contain the index. Such
> +  * index needs to be multiplied by 8.

... because .

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor

2017-03-07 Thread Ricardo Neri 
The segment descriptor contains information that is relevant to how linear
address need to be computed. It contains the default size of addresses as
well as the base address of the segment. Thus, given a segment selector,
we ought look at segment descriptor to correctly calculate the linear
address.

In protected mode, the segment selector might indicate a segment
descriptor from either the global descriptor table or a local descriptor
table. Both cases are considered in this function.

This function is the initial implementation for subsequent functions that
will obtain the aforementioned attributes of the segment descriptor.

Cc: Dave Hansen 
Cc: Adam Buchbinder 
Cc: Colin Ian King 
Cc: Lorenzo Stoakes 
Cc: Qiaowei Ren 
Cc: Arnaldo Carvalho de Melo 
Cc: Masami Hiramatsu 
Cc: Adrian Hunter 
Cc: Kees Cook 
Cc: Thomas Garnier 
Cc: Peter Zijlstra 
Cc: Borislav Petkov 
Cc: Dmitry Vyukov 
Cc: Ravi V. Shankar 
Cc: x...@kernel.org
Signed-off-by: Ricardo Neri 
---
 arch/x86/lib/insn-eval.c | 61 
 1 file changed, 61 insertions(+)

diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 8d45df8..8608adf 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -5,9 +5,13 @@
  */
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 
 enum reg_type {
@@ -294,6 +298,63 @@ static int get_reg_offset(struct insn *insn, struct 
pt_regs *regs,
 }
 
 /**
+ * get_desc() - Obtain address of segment descriptor
+ * @seg:   Segment selector
+ * @desc:  Pointer to the selected segment descriptor
+ *
+ * Given a segment selector, obtain a memory pointer to the segment
+ * descriptor. Both global and local descriptor tables are supported.
+ * desc will contain the address of the descriptor.
+ *
+ * Return: 0 if success, -EINVAL if failure
+ */
+static int get_desc(unsigned short seg, struct desc_struct **desc)
+{
+   struct desc_ptr gdt_desc = {0, 0};
+   unsigned long desc_base;
+
+   if (!desc)
+   return -EINVAL;
+
+   desc_base = seg & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);
+
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
+   if ((seg & SEGMENT_TI_MASK) == SEGMENT_LDT) {
+   seg >>= 3;
+
+   mutex_lock(>active_mm->context.lock);
+   if (unlikely(!current->active_mm->context.ldt ||
+seg >= current->active_mm->context.ldt->size)) {
+   *desc = NULL;
+   mutex_unlock(>active_mm->context.lock);
+   return -EINVAL;
+   }
+
+   *desc = >active_mm->context.ldt->entries[seg];
+   mutex_unlock(>active_mm->context.lock);
+   return 0;
+   }
+#endif
+   native_store_gdt(_desc);
+
+   /*
+* Bits [15:3] of the segment selector contain the index. Such
+* index needs to be multiplied by 8. However, as the index
+* least significant bit is already in bit 3, we don't have
+* to perform the multiplication.
+*/
+   desc_base = seg & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK);
+
+   if (desc_base > gdt_desc.size) {
+   *desc = NULL;
+   return -EINVAL;
+   }
+
+   *desc = (struct desc_struct *)(gdt_desc.address + desc_base);
+   return 0;
+}
+
+/**
  * insn_get_reg_offset_modrm_rm - Obtain register in r/m part of ModRM byte
  * @insn:  Instruction structure containing the ModRM byte
  * @regs:  Set of registers indicated by the ModRM byte
-- 
2.9.3

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