From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751851AbdJLJst (ORCPT ); Thu, 12 Oct 2017 05:48:49 -0400 Received: from mx2.suse.de ([195.135.220.15]:46831 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750758AbdJLJsr (ORCPT ); Thu, 12 Oct 2017 05:48:47 -0400 Date: Thu, 12 Oct 2017 11:48:33 +0200 From: Borislav Petkov To: Ricardo Neri 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 Subject: Re: [PATCH v9 13/29] x86/insn-eval: Add utility functions to get segment selector Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1507770750.17492.34.camel@linux.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. :-\ 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(). 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. 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. Thx. --- diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c index e7e82b343bd0..3c65fa9178bc 100644 --- a/arch/x86/lib/insn-eval.c +++ b/arch/x86/lib/insn-eval.c @@ -71,9 +71,6 @@ static int get_seg_reg_override_idx(struct insn *insn) int idx = INAT_SEG_REG_DEFAULT; int num_overrides = 0, i; - if (!insn) - return -EINVAL; - insn_get_prefixes(insn); /* Look for any segment override prefixes. */ @@ -128,28 +125,16 @@ static int get_seg_reg_override_idx(struct insn *insn) * * Returns: * - * 1 if segment override prefixes can be used with the register indicated - * in regoff. 0 if otherwise. + * True if segment override prefixes can be used with the register indicated + * in regoff. False if otherwise. * - * -EINVAL in case of error. */ -static int check_seg_overrides(struct insn *insn, int regoff) +static bool check_seg_overrides(struct insn *insn, int regoff) { - /* - * Segment override prefixes should not be used for rIP. It is not - * necessary to inspect the instruction structure. - */ - if (regoff == offsetof(struct pt_regs, ip)) - return 0; - - /* Subsequent checks require a valid insn. */ - if (!insn) - return -EINVAL; - if (regoff == offsetof(struct pt_regs, di) && is_string_insn(insn)) - return 0; + return false; - return 1; + return true; } static int resolve_default_seg(struct insn *insn, struct pt_regs *regs, int off) @@ -247,18 +232,21 @@ static int resolve_default_seg(struct insn *insn, struct pt_regs *regs, int off) */ static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff) { - int ret, idx; + int idx; - ret = check_seg_overrides(insn, regoff); - if (ret < 0) - return ret; - - if (!ret) - return resolve_default_seg(insn, regs, regoff); + /* + * Segment override prefixes should not be used for rIP. It is not + * necessary to inspect the instruction. + */ + if (regoff == offsetof(struct pt_regs, ip)) + return INAT_SEG_REG_CS; if (!insn) return -EINVAL; + if (!check_seg_overrides(insn, regoff)) + return resolve_default_seg(insn, regs, regoff); + idx = get_seg_reg_override_idx(insn); if (idx < 0) return idx; > Could this check be removed? insn is not used for anything but passed to other > functions that do perform this check. Yap, eventually. :) -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --