Re: [PATCH v7 10/26] x86/insn-eval: Add utility functions to get segment selector
On Thu, Jun 15, 2017 at 12:04:21PM -0700, Ricardo Neri wrote: > On Thu, 2017-06-15 at 11:37 -0700, Ricardo Neri wrote: > > > Yuck, didn't we talk about this already? > > > > I am sorry Borislav. I thought you agreed that I could use the values > > of > > the segment override prefixes to identify the segment registers [1]. Yes, I agreed with that but... > This time with the reference: > [1]. https://lkml.org/lkml/2017/5/5/377 ... this says it already: "... but you should call them what they are: "enum seg_override_pfxs" or "enum seg_ovr_pfx" or..." IOW, those are segment *override* prefixes and should be called such and not "enum segment_register" as this way is misleading. IOW, here's what I think you should do: /* Segment override prefixes: */ #define SEG_CS_OVERRIDE 0x23 #define SEG_SS_OVERRIDE 0x36 #define SEG_DS_OVERRIDE 0x3e ... and so on... and use the defines directly. The enum is fine and dandy but then you need to return an error value too so you can just as well have the function return an int simply and make sure you check the retval. -- 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: [PATCH v7 10/26] x86/insn-eval: Add utility functions to get segment selector
On Thu, Jun 15, 2017 at 11:37:51AM -0700, Ricardo Neri wrote: > Wouldn't this be ending up mixing the actual segment register and > segment register overrides? I plan to have a function that parses the > segment override prefixes and returns SEG_REG_CS/DS/ES/FS/GS or > SEG_REG_IGNORE for long mode or SEG_REG_DEFAULT when the default segment > register needs to be used. A separate function will determine what such > default segment register is. Does this make sense? Yes. -- 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: [PATCH v7 10/26] x86/insn-eval: Add utility functions to get segment selector
On Thu, 2017-06-15 at 11:37 -0700, Ricardo Neri wrote: > > Yuck, didn't we talk about this already? > > I am sorry Borislav. I thought you agreed that I could use the values > of > the segment override prefixes to identify the segment registers [1]. This time with the reference: [1]. https://lkml.org/lkml/2017/5/5/377 -- 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: [PATCH v7 10/26] x86/insn-eval: Add utility functions to get segment selector
On Tue, 2017-05-30 at 12:35 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:08AM -0700, 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 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 (code segments are always selected by the CS
> > segment register; string instructions always use the ES segment register
> > along with the EDI register).
> >
> > For clarity, this process can be split into two steps: resolving the
> > relevant segment register to use and, once known, read its value to
> > obtain the 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
> > the respective segment registers.
> >
> > Lastly, the only two segment registers that are not ignored in long mode
> > are FS and GS. In these two cases, base addresses are obtained from the
> > respective MSRs.
> >
> > 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 | 256
> > +++
> > 1 file changed, 256 insertions(+)
> >
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index 1634762..0a496f4 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -9,6 +9,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > enum reg_type {
> > REG_TYPE_RM = 0,
> > @@ -33,6 +34,17 @@ enum string_instruction {
> > SCASW_SCASD = 0xaf,
> > };
> >
> > +enum segment_register {
> > + SEG_REG_INVAL = -1,
> > + SEG_REG_IGNORE = 0,
> > + SEG_REG_CS = 0x23,
> > + SEG_REG_SS = 0x36,
> > + SEG_REG_DS = 0x3e,
> > + SEG_REG_ES = 0x26,
> > + SEG_REG_FS = 0x64,
> > + SEG_REG_GS = 0x65,
> > +};
>
> Yuck, didn't we talk about this already?
I am sorry Borislav. I thought you agreed that I could use the values of
the segment override prefixes to identify the segment registers [1].
>
> Those are segment override prefixes so call them as such.
>
> #define SEG_OVR_PFX_CS0x23
> #define SEG_OVR_PFX_SS0x36
> ...
>
> and we already have those!
>
> arch/x86/include/asm/inat.h:
> ...
> #define INAT_PFX_CS 5 /* 0x2E */
> #define INAT_PFX_DS 6 /* 0x3E */
> #define INAT_PFX_ES 7 /* 0x26 */
> #define INAT_PFX_FS 8 /* 0x64 */
> #define INAT_PFX_GS 9 /* 0x65 */
> #define INAT_PFX_SS 10 /* 0x36 */
>
> well, kinda, they're numbers there and not the actual prefix values.
These numbers can 'translated' to the actual value of the prefixes via
inat_get_opcode_attribute(). In my next version I am planning to use
these function and reuse the aforementioned definitions.
>
> And then there's:
>
> arch/x86/kernel/uprobes.c::is_prefix_bad() which looks at some of those.
>
> Please add your defines to inat.h
Will do.
> and make that function is_prefix_bad()
> use them instead of naked numbers. We need to pay attention to all those
> different things needing to look at insn opcodes and not let them go
> unwieldy by each defining and duplicating stuff.
I have implemented this change and will be part of my next version.
>
> > /**
> > * is_string_instruction - Determine if instruction is a string instruction
> > * @insn: Instruction structure containing the opcode
> > @@ -83,6 +95,250 @@ static bool is_string_instruction(struct insn *insn)
> > }
> > }
> >
> > +/**
> > + * resolve_seg_register() - obtain segment register
>
> That function is still returning the segment override prefix and we use
> *that* to determine the segment register.
Once
Re: [PATCH v7 10/26] x86/insn-eval: Add utility functions to get segment selector
On Fri, May 05, 2017 at 11:17:08AM -0700, 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 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 (code segments are always selected by the CS
> segment register; string instructions always use the ES segment register
> along with the EDI register).
>
> For clarity, this process can be split into two steps: resolving the
> relevant segment register to use and, once known, read its value to
> obtain the 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
> the respective segment registers.
>
> Lastly, the only two segment registers that are not ignored in long mode
> are FS and GS. In these two cases, base addresses are obtained from the
> respective MSRs.
>
> 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 | 256
> +++
> 1 file changed, 256 insertions(+)
>
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 1634762..0a496f4 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> enum reg_type {
> REG_TYPE_RM = 0,
> @@ -33,6 +34,17 @@ enum string_instruction {
> SCASW_SCASD = 0xaf,
> };
>
> +enum segment_register {
> + SEG_REG_INVAL = -1,
> + SEG_REG_IGNORE = 0,
> + SEG_REG_CS = 0x23,
> + SEG_REG_SS = 0x36,
> + SEG_REG_DS = 0x3e,
> + SEG_REG_ES = 0x26,
> + SEG_REG_FS = 0x64,
> + SEG_REG_GS = 0x65,
> +};
Yuck, didn't we talk about this already?
Those are segment override prefixes so call them as such.
#define SEG_OVR_PFX_CS 0x23
#define SEG_OVR_PFX_SS 0x36
...
and we already have those!
arch/x86/include/asm/inat.h:
...
#define INAT_PFX_CS 5 /* 0x2E */
#define INAT_PFX_DS 6 /* 0x3E */
#define INAT_PFX_ES 7 /* 0x26 */
#define INAT_PFX_FS 8 /* 0x64 */
#define INAT_PFX_GS 9 /* 0x65 */
#define INAT_PFX_SS 10 /* 0x36 */
well, kinda, they're numbers there and not the actual prefix values.
And then there's:
arch/x86/kernel/uprobes.c::is_prefix_bad() which looks at some of those.
Please add your defines to inat.h and make that function is_prefix_bad()
use them instead of naked numbers. We need to pay attention to all those
different things needing to look at insn opcodes and not let them go
unwieldy by each defining and duplicating stuff.
> /**
> * is_string_instruction - Determine if instruction is a string instruction
> * @insn:Instruction structure containing the opcode
> @@ -83,6 +95,250 @@ static bool is_string_instruction(struct insn *insn)
> }
> }
>
> +/**
> + * resolve_seg_register() - obtain segment register
That function is still returning the segment override prefix and we use
*that* to determine the segment register.
> + * @insn:Instruction structure with segment override prefixes
> + * @regs:Structure with register values as seen when entering kernel mode
> + * @regoff: Operand offset, in pt_regs, used to deterimine segment register
> + *
> + * The segment register to which an effective address refers depends on
> + * a) whether segment override prefixes must be ignored: always use CS when
> + * the register is (R|E)IP; always use ES when operand register is (E)DI with
> + * string instructions as defined in the Intel documentation. b) If segment
> + * overrides prefixes are used in the instruction instruction prefixes. C)
> Use
> + * the default segment register associated with the operand register.
> + *
> + *
