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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171013113738.757ucfevds53eg6g@pd.tnic>
Date:   Fri, 13 Oct 2017 13:37:38 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Brian Gerst <brgerst@...il.com>,
        Chris Metcalf <cmetcalf@...lanox.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Huang Rui <ray.huang@....com>, Jiri Slaby <jslaby@...e.cz>,
        Jonathan Corbet <corbet@....net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Chen Yucong <slaoub@...il.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
        x86@...nel.org, Adam Buchbinder <adam.buchbinder@...il.com>,
        Colin Ian King <colin.king@...onical.com>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        Qiaowei Ren <qiaowei.ren@...el.com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Garnier <thgarnie@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH v9 13/29] x86/insn-eval: Add utility functions to get
 segment selector

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)
--