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: <1507856897.17492.53.camel@linux.intel.com>
Date:   Thu, 12 Oct 2017 18:08:17 -0700
From:   Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To:     Borislav Petkov <bp@...e.de>
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, 2017-10-12 at 11:48 +0200, Borislav Petkov wrote:
> On Wed, Oct 11, 2017 at 06:12:30PM -0700, Ricardo Neri wrote:
> > 
> > Shouldn't this function check for a null insn since it is used here?
> I have to say, this whole codepath from insn_get_seg_base() with
> insn==NULL is nasty but I don't see a way around it as we need to know
> how many bytes to copy and from where. Can't think of a better solution
> without duplicating a lot of code. :-\

I have looked at your two proposals. I think I prefer the first one plus a
couple of tweaks.

> 
> So how about this?
> 
> If the patch is hard to read, you can apply it and look at the code. But
> here's the gist:
> 
> * You pull up the rIP check and do that directly in resolve_seg_reg()
> and return INAT_SEG_REG_CS there immediately so you don't have to call
> resolve_default_seg().

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(). Also, strictly speaking we would need to
return INAT_SEG_REG_IGNORE in long mode. Indeed, insn_get_seg_base() would
return base 0 in such a case, but I feel it is better if this logic is explicit
in resolve_default_seg().
> 
> This way, you get the only case out of the way where insn can be NULL.
> 
> Then you can do the if (!insn) check once and now you have a valid insn.

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.
> 
> check_seg_overrides() can then return simply bool and you can get rid of
> the remaining if (!insn) checks down the road.
> 
> But please double-check me if I missed a case - the flow is not trivial.

This is a diff based on your first proposal (I hope text does not wrap). I feel
this makes it clear how resolve_seg_reg() handles errors as well it uses
overridden or default segment register indices. Plus, insn is only checked when
used.

@@ -155,6 +155,16 @@ static int resolve_default_seg(struct insn *insn, struct
pt_regs *regs, int off)
 {
        if (user_64bit_mode(regs))
                return INAT_SEG_REG_IGNORE;
+
+       /*
+        * insn may be null as we may be about to copy the instruction.
+        * However is not needed at all.
+        */
+       if (off == offsetof(struct pt_regs, ip))
+               INAT_SEG_REG_CS;
+
+       if(!insn)
+               return -EINVAL;
        /*
         * If we are here, we use the default segment register as described
         * in the Intel documentation:
@@ -191,9 +201,6 @@ static int resolve_default_seg(struct insn *insn, struct
pt_regs *regs, int off)
        case offsetof(struct pt_regs, sp):
                return INAT_SEG_REG_SS;
 
-       case offsetof(struct pt_regs, ip):
-               return INAT_SEG_REG_CS;
-
        default:
                return -EINVAL;
        }
@@ -254,9 +261,6 @@ static int resolve_seg_reg(struct insn *insn, struct pt_regs
*regs, int regoff)
        if (!ret)
                return resolve_default_seg(insn, regs, regoff);
 
-       if (!insn)
-               return -EINVAL;
-
        idx = get_seg_reg_override_idx(insn);
        if (idx < 0)
                return idx;

Thanks and BR,
Ricardo