Re: [v6 PATCH 06/21] x86/insn-eval: Add utility functions to get segment selector

2017-04-30 Thread Borislav Petkov 
On Wed, Apr 26, 2017 at 01:44:43PM -0700, Ricardo Neri wrote:
> I regard that the role of this function is to obtain the the segment
> selector from either of the prefixes or inferred from the operands. It
> is the role of caller to determine if the segment selector should be
> ignored.

No, this is wrong. The function is called resolve_seg_selector() and it
gives you the segment selector. CS, DS, ES, and SS in 64-bit mode are
treated as null segments and your function should return/signal exactly
that, i.e, saying that those should be ignored in that case.

> I double-checked the latest version of the Intel Software Development
> manual [2], in the table 3-5 in section 3.7.4 mentions that DS is
> default segment for all data references, except string destinations. I
> tested this code with the UMIP-protected instructions and whenever I use
> %edi the default segment is %ds.

Yes, all correct. Except that we're adding a more-or-less generic x86
insn decoder so we should make it so...

> Is this example valid? The documentation of MOVS specifies that it
> always moves DS:(E)SI to ES:(E)DI.

... that the decoder should do exactly that:

if (MOVS and rDI)
return SEG_ES;

And you're handing in struct insn * so you can easily check which insn
you're looking at.

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 06/21] x86/insn-eval: Add utility functions to get segment selector

2017-04-26 Thread Ricardo Neri 
On Wed, 2017-04-26 at 13:44 -0700, Ricardo Neri wrote:
> > 
> > > +*/
> > > +   for (i = 0; i < insn->prefixes.nbytes; i++) {
> > > +   switch (insn->prefixes.bytes[i]) {
> > > +   case SEG_CS:
> > > +   return SEG_CS;
> > > +   case SEG_SS:
> > > +   return SEG_SS;
> > > +   case SEG_DS:
> > > +   return SEG_DS;
> > > +   case SEG_ES:
> > > +   return SEG_ES;
> > > +   case SEG_FS:
> > > +   return SEG_FS;
> > > +   case SEG_GS:
> > > +   return SEG_GS;
> > 
> > So what happens if you're in 64-bit mode and you have CS, DS, ES, or
> SS?
> > Or is this what @get_default is supposed to do? But it doesn't look
> like
> > it, it still returns segments ignored in 64-bit mode.
> 
> I regard that the role of this function is to obtain the the segment
> selector from either of the prefixes or inferred from the operands. It
> is the role of caller to determine if the segment selector should be
> ignored. So far the only caller is insn_get_seg_base() [1]. If in long
> mode, the segment base address is regarded as 0 unless the segment
> selector is FS or GS.
> > 
> > > +   default:
> > > +   return -EINVAL;
> > > +   }
> > > +   }
> > > +
> > > +default_seg:
> > > +   /*
> > > +* If no overrides, use default selectors as described in the
> > > +* Intel documentation: SS for ESP or EBP. DS for all data
> references,
> > > +* except when relative to stack or string destination.
> > > +* Also, AX, CX and DX are not valid register operands in
> 16-bit
> > > +* address encodings.
> > > +* Callers must interpret the result correctly according to
> the type
> > > +* of instructions (e.g., use ES for string instructions).
> > > +* Also, some values of modrm and sib might seem to indicate
> the use
> > > +* of EBP and ESP (e.g., modrm_mod = 0, modrm_rm = 5) but
> actually
> > > +* they refer to cases in which only a displacement used.
> These cases
> > > +* should be indentified by the caller and not with this
> function.
> > > +*/
> > > +   switch (regoff) {
> > > +   case offsetof(struct pt_regs, ax):
> > > +   /* fall through */
> > > +   case offsetof(struct pt_regs, cx):
> > > +   /* fall through */
> > > +   case offsetof(struct pt_regs, dx):
> > > +   if (insn && insn->addr_bytes == 2)
> > > +   return -EINVAL;
> > > +   case -EDOM: /* no register involved in address computation */
> > > +   case offsetof(struct pt_regs, bx):
> > > +   /* fall through */
> > > +   case offsetof(struct pt_regs, di):
> > > +   /* fall through */
> > 
> >   return SEG_ES;
> > 
> > ?
> 
> I double-checked the latest version of the Intel Software Development
> manual [2], in the table 3-5 in section 3.7.4 mentions that DS is
> default segment for all data references, except string destinations. I
> tested this code with the UMIP-protected instructions and whenever I
> use
> %edi the default segment is %ds.


I forgot my references:

[1]. https://lkml.org/lkml/2017/3/7/876
[2]. https://software.intel.com/en-us/articles/intel-sdm#combined

--
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 06/21] x86/insn-eval: Add utility functions to get segment selector

2017-04-26 Thread Ricardo Neri 
On Tue, 2017-04-18 at 11:42 +0200, Borislav Petkov wrote:
> On Tue, Mar 07, 2017 at 04:32:39PM -0800, Ricardo Neri wrote:
> > When computing a linear address and segmentation is used, we need to know
> > the base address of the segment involved in the computation. In most of
> > the cases, the segment base address will be zero as in USER_DS/USER32_DS.
> > However, it may be possible that a user space program defines its own
> > segments via a local descriptor table. In such a case, the segment base
> > address may not be zero .Thus, the segment base address is needed to
> > calculate correctly the linear address.
> > 
> > The segment selector to be used when computing a linear address is
> > determined by either any of segment select override prefixes in the
> > instruction or inferred from the registers involved in the computation of
> > the effective address; in that order. Also, there are cases when the
> > overrides shall be ignored.
> > 
> > For clarity, this process can be split into two steps: resolving the
> > relevant segment and, once known, read the applicable segment selector.
> > The method to obtain the segment selector depends on several factors. In
> > 32-bit builds, segment selectors are saved into the pt_regs structure
> > when switching to kernel mode. The same is also true for virtual-8086
> > mode. In 64-bit builds, segmentation is mostly ignored, except when
> > running a program in 32-bit legacy mode. In this case, CS and SS can be
> > obtained from pt_regs. DS, ES, FS and GS can be read directly from
> > registers.
> 
> > Lastly, segmentation is possible in 64-bit mode via FS and GS.
> 
> I'd say "Lastly, the only two segment registers which are not ignored in
> long mode are FS and GS."

I will make this clarification.
> 
> > In these two cases, base addresses are obtained from the relevant MSRs.
> 
> s/relevant/respective/

Will clarify.
> 
> > 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 | 195 
> > +++
> >  1 file changed, 195 insertions(+)
> > 
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index 78df1c9..8d45df8 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -8,6 +8,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  enum reg_type {
> > REG_TYPE_RM = 0,
> > @@ -15,6 +16,200 @@ enum reg_type {
> > REG_TYPE_BASE,
> >  };
> >  
> > +enum segment {
> > +   SEG_CS = 0x23,
> > +   SEG_SS = 0x36,
> > +   SEG_DS = 0x3e,
> > +   SEG_ES = 0x26,
> > +   SEG_FS = 0x64,
> > +   SEG_GS = 0x65
> > +};
> > +
> > +/**
> > + * resolve_seg_selector() - obtain segment selector
> > + * @regs:  Set of registers containing the segment selector
> 
> That arg is gone.

This came from one of my initial implementations. I will remove it.
> 
> > + * @insn:  Instruction structure with selector override prefixes
> > + * @regoff:Operand offset, in pt_regs, of which the selector is 
> > needed
> > + * @default:   Resolve default segment selector (i.e., ignore 
> > overrides)
> > + *
> > + * The segment selector to which an effective address refers depends on
> > + * a) segment selector overrides instruction prefixes or b) the operand
> > + * register indicated in the ModRM or SiB byte.
> > + *
> > + * For case a), the function inspects any prefixes in the insn instruction;
> 
> s/insn //

In this case I meant "any prefixes in the insn structure". Probably it
will make it more clear.
> 
> > + * insn can be null to indicate that selector override prefixes shall be
> > + * ignored.
> 
> This is not what the code does: it returns -EINVAL when insn is NULL.

This was the behavior in a previous implementation. I will update it.
> 
> > This is useful when the use of prefixes is forbidden (e.g.,
> > + * obtaining the code selector). For case b), the operand register shall be
> > + * represented as the offset from the base address of pt_regs. Also, regoff
> > + * can be -EINVAL for cases in which registers are not used as operands 
> > (e.g.,
> > + * when the mod and r/m parts of the ModRM byte are 0 and 5, respectively).
> > + *
> > + * This function returns the segment selector to utilize as per the 
> > conditions
> > + * described above.

Re: [v6 PATCH 06/21] x86/insn-eval: Add utility functions to get segment selector

2017-04-18 Thread Borislav Petkov 
On Tue, Mar 07, 2017 at 04:32:39PM -0800, Ricardo Neri wrote:
> When computing a linear address and segmentation is used, we need to know
> the base address of the segment involved in the computation. In most of
> the cases, the segment base address will be zero as in USER_DS/USER32_DS.
> However, it may be possible that a user space program defines its own
> segments via a local descriptor table. In such a case, the segment base
> address may not be zero .Thus, the segment base address is needed to
> calculate correctly the linear address.
> 
> The segment selector to be used when computing a linear address is
> determined by either any of segment select override prefixes in the
> instruction or inferred from the registers involved in the computation of
> the effective address; in that order. Also, there are cases when the
> overrides shall be ignored.
> 
> For clarity, this process can be split into two steps: resolving the
> relevant segment and, once known, read the applicable segment selector.
> The method to obtain the segment selector depends on several factors. In
> 32-bit builds, segment selectors are saved into the pt_regs structure
> when switching to kernel mode. The same is also true for virtual-8086
> mode. In 64-bit builds, segmentation is mostly ignored, except when
> running a program in 32-bit legacy mode. In this case, CS and SS can be
> obtained from pt_regs. DS, ES, FS and GS can be read directly from
> registers.

> Lastly, segmentation is possible in 64-bit mode via FS and GS.

I'd say "Lastly, the only two segment registers which are not ignored in
long mode are FS and GS."

> In these two cases, base addresses are obtained from the relevant MSRs.

s/relevant/respective/

> 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 | 195 
> +++
>  1 file changed, 195 insertions(+)
> 
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 78df1c9..8d45df8 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -8,6 +8,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  enum reg_type {
>   REG_TYPE_RM = 0,
> @@ -15,6 +16,200 @@ enum reg_type {
>   REG_TYPE_BASE,
>  };
>  
> +enum segment {
> + SEG_CS = 0x23,
> + SEG_SS = 0x36,
> + SEG_DS = 0x3e,
> + SEG_ES = 0x26,
> + SEG_FS = 0x64,
> + SEG_GS = 0x65
> +};
> +
> +/**
> + * resolve_seg_selector() - obtain segment selector
> + * @regs:Set of registers containing the segment selector

That arg is gone.

> + * @insn:Instruction structure with selector override prefixes
> + * @regoff:  Operand offset, in pt_regs, of which the selector is needed
> + * @default: Resolve default segment selector (i.e., ignore overrides)
> + *
> + * The segment selector to which an effective address refers depends on
> + * a) segment selector overrides instruction prefixes or b) the operand
> + * register indicated in the ModRM or SiB byte.
> + *
> + * For case a), the function inspects any prefixes in the insn instruction;

s/insn //

> + * insn can be null to indicate that selector override prefixes shall be
> + * ignored.

This is not what the code does: it returns -EINVAL when insn is NULL.

> This is useful when the use of prefixes is forbidden (e.g.,
> + * obtaining the code selector). For case b), the operand register shall be
> + * represented as the offset from the base address of pt_regs. Also, regoff
> + * can be -EINVAL for cases in which registers are not used as operands 
> (e.g.,
> + * when the mod and r/m parts of the ModRM byte are 0 and 5, respectively).
> + *
> + * This function returns the segment selector to utilize as per the 
> conditions
> + * described above. Please note that this functin does not return the value
> + * of the segment selector. The value of the segment selector needs to be
> + * obtained using get_segment_selector and passing the segment selector type
> + * resolved by this function.
> + *
> + * Return: Segment selector to use, among CS, SS, DS, ES, FS or GS.

: negative value when...

> + */
> +static int resolve_seg_selector(struct insn *insn, int regoff, bool 
> get_default)
> +{
> + int i;
> +
> + if (!insn)
> + return -EINVAL;
> +
> + if (get_default)
> +