From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755605AbdJMBIz (ORCPT ); Thu, 12 Oct 2017 21:08:55 -0400 Received: from mga14.intel.com ([192.55.52.115]:28055 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753303AbdJMBIy (ORCPT ); Thu, 12 Oct 2017 21:08:54 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,369,1503385200"; d="scan'208";a="159959715" Message-ID: <1507856897.17492.53.camel@linux.intel.com> Subject: Re: [PATCH v9 13/29] x86/insn-eval: Add utility functions to get segment selector From: Ricardo Neri To: Borislav Petkov Cc: Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , Andy Lutomirski , Peter Zijlstra , Andrew Morton , Brian Gerst , Chris Metcalf , Dave Hansen , Paolo Bonzini , Masami Hiramatsu , Huang Rui , Jiri Slaby , Jonathan Corbet , "Michael S. Tsirkin" , Paul Gortmaker , Vlastimil Babka , Chen Yucong , "Ravi V. Shankar" , Shuah Khan , linux-kernel@vger.kernel.org, x86@kernel.org, Adam Buchbinder , Colin Ian King , Lorenzo Stoakes , Qiaowei Ren , Arnaldo Carvalho de Melo , Adrian Hunter , Kees Cook , Thomas Garnier , Dmitry Vyukov Date: Thu, 12 Oct 2017 18:08:17 -0700 In-Reply-To: <20171012094833.xjk6jg5us3ffkih4@pd.tnic> References: <1507089272-32733-1-git-send-email-ricardo.neri-calderon@linux.intel.com> <1507089272-32733-14-git-send-email-ricardo.neri-calderon@linux.intel.com> <20171010224144.wedursa2s3zpip43@pd.tnic> <1507770750.17492.34.camel@linux.intel.com> <20171012094833.xjk6jg5us3ffkih4@pd.tnic> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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