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

2017-05-11 Thread Ricardo Neri 
On Fri, 2017-05-05 at 19:19 +0200, Borislav Petkov wrote:
> On Wed, Apr 26, 2017 at 03:37:44PM -0700, Ricardo Neri wrote:
> > I need a human-readable way of identifying what segment selector (in
> > pt_regs, vm86regs or directly reading the segment registers) to use.
> > Since there is a segment override prefix for all of them, I thought I
> > could use them.
> 
> Yes, you should...
> 
> > Perhaps I can rename enum segment to enum segment_selector and comment
> > that the values in the enum are those of the override prefixes. Would
> > that be reasonable?
> 
> ... but you should call them what they are: "enum seg_override_pfxs" or
> "enum seg_ovr_pfx" or...
> 
> Or somesuch. I suck at naming stuff.

In my v7, I simply named my enumeration enum segment_register, which is
what they are. Some of its entries happen to have the value of the
segment override prefixes but also have special entries as SEG_REG_INVAL
when for errors and SEG_REG_IGNORE for long mode [1].

Thanks and BR,
Ricardo

[1]. https://lkml.org/lkml/2017/5/5/405

--
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 08/21] x86/insn-eval: Add utility function to get segment descriptor base address

2017-05-11 Thread Ricardo Neri 
On Fri, 2017-05-05 at 19:28 +0200, Borislav Petkov wrote:
> On Wed, Apr 26, 2017 at 03:52:41PM -0700, Ricardo Neri wrote:
> > Probably insn_get_seg_base() itself can verify if there are segment
> > override prefixes in the struct insn. If yes, use them except for
> > specific cases such as CS.
> 
> ... and depending on whether in long mode or not.

Yes, in my v7 I ignore the segment register if we are in long mode [1].
> 
> > On an unrelated note, I still have the problem of using DS vs ES for
> > string instructions. Perhaps instead of a use_default_seg flag, a
> > string_instruction flag that indicates how to determine the default
> > segment.
> 
> ... or you can look at the insn opcode directly. AFAICT, you need
> to check whether the opcode is 0xa4 or 0xa5 and that the insn is a
> single-byte opcode, i.e., not from the secondary map escaped with 0xf or
> some of the other multi-byte opcode maps.

In my v7, I have added a section my function resolve_seg_register() that
ignores
segment overrides if it sees string instructions and the register EDI
and defaults to ES. If the register is EIP, it defaults to CS. To
determine if an instruction is a string instruction I do check for the
size of the opcode and the opcodes that you mention plus others based on
the Intel Software Development Manual[2].

[1]. https://lkml.org/lkml/2017/5/5/405
[2]. https://lkml.org/lkml/2017/5/5/410

Thanks and BR,
Ricardo


> 
> -- 
> 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 08/21] x86/insn-eval: Add utility function to get segment descriptor base address

2017-05-05 Thread Borislav Petkov 
On Wed, Apr 26, 2017 at 03:52:41PM -0700, Ricardo Neri wrote:
> Probably insn_get_seg_base() itself can verify if there are segment
> override prefixes in the struct insn. If yes, use them except for
> specific cases such as CS.

... and depending on whether in long mode or not.

> On an unrelated note, I still have the problem of using DS vs ES for
> string instructions. Perhaps instead of a use_default_seg flag, a
> string_instruction flag that indicates how to determine the default
> segment.

... or you can look at the insn opcode directly. AFAICT, you need
to check whether the opcode is 0xa4 or 0xa5 and that the insn is a
single-byte opcode, i.e., not from the secondary map escaped with 0xf or
some of the other multi-byte opcode maps.

-- 
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 08/21] x86/insn-eval: Add utility function to get segment descriptor base address

2017-05-05 Thread Borislav Petkov 
On Wed, Apr 26, 2017 at 03:37:44PM -0700, Ricardo Neri wrote:
> I need a human-readable way of identifying what segment selector (in
> pt_regs, vm86regs or directly reading the segment registers) to use.
> Since there is a segment override prefix for all of them, I thought I
> could use them.

Yes, you should...

> Perhaps I can rename enum segment to enum segment_selector and comment
> that the values in the enum are those of the override prefixes. Would
> that be reasonable?

... but you should call them what they are: "enum seg_override_pfxs" or
"enum seg_ovr_pfx" or...

Or somesuch. I suck at naming stuff.

-- 
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 08/21] x86/insn-eval: Add utility function to get segment descriptor base address

2017-04-26 Thread Ricardo Neri 
On Thu, 2017-04-20 at 10:25 +0200, Borislav Petkov wrote:
> > + * insn_get_seg_base() - Obtain base address contained in
> descriptor
> > + * @regs:Set of registers containing the segment selector
> > + * @insn:Instruction structure with selector override prefixes
> > + * @regoff:  Operand offset, in pt_regs, of which the selector is
> needed
> > + * @use_default_seg: Use the default segment instead of prefix
> overrides
> 
> I'm wondering whether you really need that bool or you can deduce this
> from pt_regs... I guess I'll see...

Probably insn_get_seg_base() itself can verify if there are segment
override prefixes in the struct insn. If yes, use them except for
specific cases such as CS.

On an unrelated note, I still have the problem of using DS vs ES for
string instructions. Perhaps instead of a use_default_seg flag, a
string_instruction flag that indicates how to determine the default
segment.

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 08/21] x86/insn-eval: Add utility function to get segment descriptor base address

2017-04-26 Thread Ricardo Neri 
On Thu, 2017-04-20 at 10:25 +0200, Borislav Petkov wrote:
> On Tue, Mar 07, 2017 at 04:32:41PM -0800, Ricardo Neri wrote:
> > With segmentation, the base address of the segment descriptor is needed
> > to compute a linear address. The segment descriptor used in the address
> > computation depends on either any segment override prefixes in the in the
> 
> s/in the //

I will fix this typo.
> 
> > instruction or the default segment determined by the registers involved
> > in the address computation. Thus, both the instruction as well as the
> > register (specified as the offset from the base of pt_regs) are given as
> > inputs, along with a boolean variable to select between override and
> > default.
> > 
> > The segment selector is determined by get_seg_selector with the inputs
> 
> Please end function names with parentheses: get_seg_selector().

I will use parentheses.
> 
> > described above. Once the selector is known the base address is
> 
>   known, ...

Will fix.
> 
> > determined. In protected mode, the selector is used to obtain the segment
> > descriptor and then its base address. If in 64-bit user mode, the segment =
> > base address is zero except when FS or GS are used. In virtual-8086 mode,
> > the base address is computed as the value of the segment selector shifted 4
> > positions to the left.
> 
> Good.
> 
> > 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/include/asm/insn-eval.h |  2 ++
> >  arch/x86/lib/insn-eval.c | 66 
> > 
> >  2 files changed, 68 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/insn-eval.h 
> > b/arch/x86/include/asm/insn-eval.h
> > index 754211b..b201742 100644
> > --- a/arch/x86/include/asm/insn-eval.h
> > +++ b/arch/x86/include/asm/insn-eval.h
> > @@ -15,5 +15,7 @@ void __user *insn_get_addr_ref(struct insn *insn, struct 
> > pt_regs *regs);
> >  int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs);
> >  int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs);
> >  int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs);
> > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn,
> > +   int regoff, bool use_default_seg);
> >  
> >  #endif /* _ASM_X86_INSN_EVAL_H */
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index 8608adf..383ca83 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -355,6 +355,72 @@ static int get_desc(unsigned short seg, struct 
> > desc_struct **desc)
> >  }
> >  
> >  /**
> > + * insn_get_seg_base() - Obtain base address contained in descriptor
> > + * @regs:  Set of registers containing the segment selector
> > + * @insn:  Instruction structure with selector override prefixes
> > + * @regoff:Operand offset, in pt_regs, of which the selector is 
> > needed
> > + * @use_default_seg: Use the default segment instead of prefix overrides
> 
> I'm wondering whether you really need that bool or you can deduce this
> from pt_regs... I guess I'll see...
> 
> > + *
> > + * Obtain the base address of the segment descriptor as indicated by either
> > + * any segment override prefixes contained in insn or the default segment
> > + * applicable to the register indicated by regoff. regoff is specified as 
> > the
> > + * offset in bytes from the base of pt_regs.
> > + *
> > + * Return: In protected mode, base address of the segment. It may be zero 
> > in
> > + * certain cases for 64-bit builds and/or 64-bit applications. In 
> > virtual-8086
> > + * mode, the segment selector shifed 4 positions to the right. -1L in case 
> > of
> 
> s/shifed/shifted/

I will correct the typo.
> 
> > + * error.
> > + */
> > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn,
> > +   int regoff, bool use_default_seg)
> > +{
> > +   struct desc_struct *desc;
> > +   unsigned short seg;
> > +   enum segment seg_type;
> > +   int ret;
> > +
> > +   seg_type = resolve_seg_selector(insn, regoff, use_default_seg);
> 
> <--- error handling.

I will add it.
> 
> And that's not really a "seg_type" but simply the "sel"-ector.

I will update the variable names to reflect the fact

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

2017-04-20 Thread Borislav Petkov 
On Tue, Mar 07, 2017 at 04:32:41PM -0800, Ricardo Neri wrote:
> With segmentation, the base address of the segment descriptor is needed
> to compute a linear address. The segment descriptor used in the address
> computation depends on either any segment override prefixes in the in the

s/in the //

> instruction or the default segment determined by the registers involved
> in the address computation. Thus, both the instruction as well as the
> register (specified as the offset from the base of pt_regs) are given as
> inputs, along with a boolean variable to select between override and
> default.
> 
> The segment selector is determined by get_seg_selector with the inputs

Please end function names with parentheses: get_seg_selector().

> described above. Once the selector is known the base address is

known, ...

> determined. In protected mode, the selector is used to obtain the segment
> descriptor and then its base address. If in 64-bit user mode, the segment =
> base address is zero except when FS or GS are used. In virtual-8086 mode,
> the base address is computed as the value of the segment selector shifted 4
> positions to the left.

Good.

> 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/include/asm/insn-eval.h |  2 ++
>  arch/x86/lib/insn-eval.c | 66 
> 
>  2 files changed, 68 insertions(+)
> 
> diff --git a/arch/x86/include/asm/insn-eval.h 
> b/arch/x86/include/asm/insn-eval.h
> index 754211b..b201742 100644
> --- a/arch/x86/include/asm/insn-eval.h
> +++ b/arch/x86/include/asm/insn-eval.h
> @@ -15,5 +15,7 @@ void __user *insn_get_addr_ref(struct insn *insn, struct 
> pt_regs *regs);
>  int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs);
>  int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs);
>  int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs);
> +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn,
> + int regoff, bool use_default_seg);
>  
>  #endif /* _ASM_X86_INSN_EVAL_H */
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 8608adf..383ca83 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -355,6 +355,72 @@ static int get_desc(unsigned short seg, struct 
> desc_struct **desc)
>  }
>  
>  /**
> + * insn_get_seg_base() - Obtain base address contained in descriptor
> + * @regs:Set of registers containing the segment selector
> + * @insn:Instruction structure with selector override prefixes
> + * @regoff:  Operand offset, in pt_regs, of which the selector is needed
> + * @use_default_seg: Use the default segment instead of prefix overrides

I'm wondering whether you really need that bool or you can deduce this
from pt_regs... I guess I'll see...

> + *
> + * Obtain the base address of the segment descriptor as indicated by either
> + * any segment override prefixes contained in insn or the default segment
> + * applicable to the register indicated by regoff. regoff is specified as the
> + * offset in bytes from the base of pt_regs.
> + *
> + * Return: In protected mode, base address of the segment. It may be zero in
> + * certain cases for 64-bit builds and/or 64-bit applications. In 
> virtual-8086
> + * mode, the segment selector shifed 4 positions to the right. -1L in case of

s/shifed/shifted/

> + * error.
> + */
> +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn,
> + int regoff, bool use_default_seg)
> +{
> + struct desc_struct *desc;
> + unsigned short seg;
> + enum segment seg_type;
> + int ret;
> +
> + seg_type = resolve_seg_selector(insn, regoff, use_default_seg);

<--- error handling.

And that's not really a "seg_type" but simply the "sel"-ector. And that
"enum segment" is not really a segment but an segment override prefixes
enum. Can we please get the nomenclature right first?

> +
> + seg = get_segment_selector(regs, seg_type);

s/seg/sel/

> + if (seg < 0)
> + return -1L;
> +
> + if (v8086_mode(regs))
> + /*
> +  * Base is simply the segment selector shifted 4
> +  * positions to the right.
> +  */
> +

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

2017-03-07 Thread Ricardo Neri 
With segmentation, the base address of the segment descriptor is needed
to compute a linear address. The segment descriptor used in the address
computation depends on either any segment override prefixes in the in the
instruction or the default segment determined by the registers involved
in the address computation. Thus, both the instruction as well as the
register (specified as the offset from the base of pt_regs) are given as
inputs, along with a boolean variable to select between override and
default.

The segment selector is determined by get_seg_selector with the inputs
described above. Once the selector is known the base address is
determined. In protected mode, the selector is used to obtain the segment
descriptor and then its base address. If in 64-bit user mode, the segment =
base address is zero except when FS or GS are used. In virtual-8086 mode,
the base address is computed as the value of the segment selector shifted 4
positions to the left.

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/include/asm/insn-eval.h |  2 ++
 arch/x86/lib/insn-eval.c | 66 
 2 files changed, 68 insertions(+)

diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
index 754211b..b201742 100644
--- a/arch/x86/include/asm/insn-eval.h
+++ b/arch/x86/include/asm/insn-eval.h
@@ -15,5 +15,7 @@ void __user *insn_get_addr_ref(struct insn *insn, struct 
pt_regs *regs);
 int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs);
 int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs);
 int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs);
+unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn,
+   int regoff, bool use_default_seg);
 
 #endif /* _ASM_X86_INSN_EVAL_H */
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 8608adf..383ca83 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -355,6 +355,72 @@ static int get_desc(unsigned short seg, struct desc_struct 
**desc)
 }
 
 /**
+ * insn_get_seg_base() - Obtain base address contained in descriptor
+ * @regs:  Set of registers containing the segment selector
+ * @insn:  Instruction structure with selector override prefixes
+ * @regoff:Operand offset, in pt_regs, of which the selector is needed
+ * @use_default_seg: Use the default segment instead of prefix overrides
+ *
+ * Obtain the base address of the segment descriptor as indicated by either
+ * any segment override prefixes contained in insn or the default segment
+ * applicable to the register indicated by regoff. regoff is specified as the
+ * offset in bytes from the base of pt_regs.
+ *
+ * Return: In protected mode, base address of the segment. It may be zero in
+ * certain cases for 64-bit builds and/or 64-bit applications. In virtual-8086
+ * mode, the segment selector shifed 4 positions to the right. -1L in case of
+ * error.
+ */
+unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn,
+   int regoff, bool use_default_seg)
+{
+   struct desc_struct *desc;
+   unsigned short seg;
+   enum segment seg_type;
+   int ret;
+
+   seg_type = resolve_seg_selector(insn, regoff, use_default_seg);
+
+   seg = get_segment_selector(regs, seg_type);
+   if (seg < 0)
+   return -1L;
+
+   if (v8086_mode(regs))
+   /*
+* Base is simply the segment selector shifted 4
+* positions to the right.
+*/
+   return (unsigned long)(seg << 4);
+
+#ifdef CONFIG_X86_64
+   if (user_64bit_mode(regs)) {
+   /*
+* Only FS or GS will have a base address, the rest of
+* the segments' bases are forced to 0.
+*/
+   unsigned long base;
+
+   if (seg_type == SEG_FS)
+   rdmsrl(MSR_FS_BASE, base);
+   else if (seg_type == SEG_GS)
+   /*
+* swapgs was called at the kernel entry point. Thus,
+* MSR_KERNEL_GS_BASE will have the user-space GS base.
+*/
+   rdmsrl(MSR_KERNEL_GS_BASE, base);
+