Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector

[Ricardo Neri]

On Thu, 2017-09-28 at 11:36 +0200, Borislav Petkov wrote:
> On Wed, Sep 27, 2017 at 03:32:26PM -0700, Ricardo Neri wrote:
> > 
> > The idea is thatÂget_overridden_seg_reg() would implement the logic you
> > just described. It would return return INAT_SEG_REG_DEFAULT/IGNORE when
> > segment override prefixes are not allowed (i.e., valid insn with
> > operand rDI and string instruction; and rIP) or needed (i.e., long
> > mode, except if there are override prefixes for FS or GS); or
> > INAT_SEG_REG_[CSDEFG]S otherwise.
> Ok, lemme see if we're talking the same thing. Your diff is linewrapped
> so parsing that is hard.
> 
> Do this
> 
> ÂÂÂÂÂÂÂÂif (regoff == offsetof(struct pt_regs, ip)) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (user_64bit_mode(regs))
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn INAT_SEG_REG_IGNORE;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn INAT_SEG_REG_DEFAULT;
> ÂÂÂÂÂÂÂÂ}
> 
> and all the other checking *before* you do insn_init(). Because you have
> crazy stuff like:
> 
> ÂÂÂÂÂÂÂÂif (seg_reg == INAT_SEG_REG_IGNORE)
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn seg_reg;
> 
> which shortcuts those functions and is simply clumsy and complicates
> following the code. The mere fact that you have to call the function
> "get_overridden_seg_reg_if_any_or_needed()" already tells you that that
> function is doing too many things at once.
> 
> When the function is called get_segment_register() then it should do
> only that. And all the checking is done before or in wrappers.

Yes, I realized this while I was typing.
> 
> IOW, all the rIP checking and early return down the
> insn_get_seg_base() -> resolve_seg_register() -> .. should be done
> separately.

Agreed now.
> 
> *Then* you do insn_init() and hand it down to insn_get_seg_base() and
> from now on you have a proper insn pointer which you hand around and
> check for NULL only once, on function entry.

I agree. In fact, insn_get_seg_base() does not need insn at all. All it needs is
a INAT_SEG_REG_* index. This would make things clear. UMIP (and callers that
need to copy_from_user code can do insn_get_seg_base(regs, INAT_SEG_REG_CS). No
insn needed.

In fact, it is only the insn_get_addr_ref_xx() family of functions that does
need to inspect insn (which will be populated and valided) to determine the what
registers are used as operands... and determine the applicable segment register.

However,Âinsn_get_addr_ref_xx() functions callÂinsn_get_seg_base() several times
each. Each time they would need to do:

if (can_use_seg_override_prefixes(insn, regoff))
  idx = get_overriden_seg_reg(insn, regs)
else
  idx = get_default_seg_reg()

The pseudocode above looks like a resolve_reg_idx() to me.

Then insn_get_addr_ref_xx() can call insn_get_seg_base(idx).

> 
> Then your code flow is much simpler: first you take care of the case
> where rIP doesn't do segment overrides and all the other cases are
> handled by the normal path, with a proper struct insn.

Do you think the pseudocode above addresses your concerns?

*insn_get_seg_base() will take a INAT_SEG_REG_* index
*insn_get_ref_xx() receives an initialized insn that can check for NULL value.
*a reworked resolve_seg_reg_idx will clearly check if it can use segment
override prefixes and obtain them. If not, it will use default values.

Thanks and BR,
Ricardo