Re: [PATCH v9 13/29] x86/insn-eval: Add utility functions to get segment selector

[Borislav Petkov]

On Thu, Oct 12, 2017 at 06:08:17PM -0700, Ricardo Neri wrote:
> In my opinion it would be better to have all the checks in a single place. This
> makes the code easier to read that having this special case directly
> inÂresolve_default_seg().
> ...
> Rather than checking for null insn inÂresolve_seg_reg(), which does not use it,
> let the functions it calls do the check if they need to.

Of course it is using it - it is passing it down to callers.

No, this is completely backwards. You're pushing the if (!insn) check
down instead of up. What you wanna do instead is get that "strange" case
out of the way *first* where insn is NULL and then have the remaining
flow with a properly allocated struct insn.

And the only case where insn is NULL is fixup_umip_exception(). All the
other callers of insn_get_seg_base() supply a properly setup struct insn
* AFAICT.

So do the minimum work of getting the segment base either directly in
fixup_umip_exception() by calling a helper function as it matters only
there.

And IINM, you have two possible cases:

1. INAT_SEG_REG_IGNORE which makes segment base 0

2. INAT_SEG_REG_DEFAULT which maps to INAT_SEG_REG_CS for the rIP case
and then gets the selector:

	sel = (unsigned short)(regs->cs & 0xffff);

and then computes the base.

And the mapping of sel to base you can do by carving out the piece of
insn_get_seg_base() *after* you've computed @sel and you do the base
computation, i.e., the piece which starts with this:

        if (v8086_mode(regs))
                /*
                 * Base is simply the segment selector shifted 4
                 * positions to the right.
	...

into a separate function called __get_seg_base(sel, ...).

The important thing to note here is that this function won't need insn
so you can call it without one.

This way you have it nice and clean designed with a clear separation of
the cases *before* a valid struct insn * and *after*.

Right now, you have both intermixed and the code is hard to follow as
you have to pay attention at each time: how and where am I being called.

Makes sense?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendÃrffer, Jane Smithard, Graham Norton, HRB 21284 (AG NÃrnberg)
--