From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751215AbdFCRXQ (ORCPT ); Sat, 3 Jun 2017 13:23:16 -0400 Received: from mga03.intel.com ([134.134.136.65]:15533 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751156AbdFCRXO (ORCPT ); Sat, 3 Jun 2017 13:23:14 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,291,1493708400"; d="scan'208";a="108812306" Message-ID: <1496510592.24288.67.camel@ranerica-desktop> Subject: Re: [PATCH v7 12/26] x86/insn-eval: Add utility functions to get segment descriptor base address and limit 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 , Alexandre Julliard , Stas Sergeev , Fenghua Yu , "Ravi V. Shankar" , Shuah Khan , linux-kernel@vger.kernel.org, x86@kernel.org, linux-msdos@vger.kernel.org, wine-devel@winehq.org, Adam Buchbinder , Colin Ian King , Lorenzo Stoakes , Qiaowei Ren , Arnaldo Carvalho de Melo , Adrian Hunter , Kees Cook , Thomas Garnier , Dmitry Vyukov Date: Sat, 03 Jun 2017 10:23:12 -0700 In-Reply-To: <20170531165839.6nlkmdlrqnuloulz@pd.tnic> References: <20170505181724.55000-1-ricardo.neri-calderon@linux.intel.com> <20170505181724.55000-13-ricardo.neri-calderon@linux.intel.com> <20170531165839.6nlkmdlrqnuloulz@pd.tnic> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2017-05-31 at 18:58 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:10AM -0700, Ricardo Neri wrote: > > With segmentation, the base address of the segment descriptor is needed > > to compute a linear address. The segment descriptor used in the address > > computation depends on either any segment override prefixes in the > > instruction or the default segment determined by the registers involved > > in the address computation. Thus, both the instruction as well as the > > register (specified as the offset from the base of pt_regs) are given as > > inputs, along with a boolean variable to select between override and > > default. > > ... > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index f46cb31..c77ed80 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -476,6 +476,133 @@ static struct desc_struct *get_desc(unsigned short sel) > > } > > > > /** > > + * insn_get_seg_base() - Obtain base address of segment descriptor. > > + * @regs: Structure with register values as seen when entering kernel mode > > + * @insn: Instruction structure with selector override prefixes > > + * @regoff: Operand offset, in pt_regs, of which the selector is needed > > + * > > + * Obtain the base address of the segment descriptor as indicated by either > > + * any segment override prefixes contained in insn or the default segment > > + * applicable to the register indicated by regoff. regoff is specified as the > > + * offset in bytes from the base of pt_regs. > > + * > > + * Return: In protected mode, base address of the segment. Zero in for long > > + * mode, except when FS or GS are used. In virtual-8086 mode, the segment > > + * selector shifted 4 positions to the right. -1L in case of > > + * error. > > + */ > > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > > + int regoff) > > +{ > > + struct desc_struct *desc; > > + unsigned short sel; > > + enum segment_register seg_reg; > > + > > + seg_reg = resolve_seg_register(insn, regs, regoff); > > + if (seg_reg == SEG_REG_INVAL) > > + return -1L; > > + > > + sel = get_segment_selector(regs, seg_reg); > > + if ((short)sel < 0) > > I guess it would be better if that function returned a signed short so > you don't have to cast it here. (You're casting it to an unsigned long > below anyway.) Yes, this make sense. I will make this change. > > > + return -1L; > > + > > + if (v8086_mode(regs)) > > + /* > > + * Base is simply the segment selector shifted 4 > > + * positions to the right. > > + */ > > + return (unsigned long)(sel << 4); > > + > > ... > > > +static unsigned long get_seg_limit(struct pt_regs *regs, struct insn *insn, > > + int regoff) > > +{ > > + struct desc_struct *desc; > > + unsigned short sel; > > + unsigned long limit; > > + enum segment_register seg_reg; > > + > > + seg_reg = resolve_seg_register(insn, regs, regoff); > > + if (seg_reg == SEG_REG_INVAL) > > + return 0; > > + > > + sel = get_segment_selector(regs, seg_reg); > > + if ((short)sel < 0) > > Ditto. Here as well. > > > + return 0; > > + > > + if (user_64bit_mode(regs) || v8086_mode(regs)) > > + return -1L; > > + > > + if (!sel) > > + return 0; > > + > > + desc = get_desc(sel); > > + if (!desc) > > + return 0; > > + > > + /* > > + * If the granularity bit is set, the limit is given in multiples > > + * of 4096. When the granularity bit is set, the least 12 significant > > the 12 least significant bits > > > + * bits are not tested when checking the segment limits. In practice, > > + * this means that the segment ends in (limit << 12) + 0xfff. > > + */ > > + limit = get_desc_limit(desc); > > + if (desc->g) > > + limit <<= 12 | 0x7; > > That 0x7 doesn't look like 0xfff - it shifts limit by 15 instead. You > can simply write it like you mean it: > > limit = (limit << 12) + 0xfff; You are right, this wrong. I will implement as you mention. Thanks and BR, Ricardo