lkml.org 
[lkml]   [2017]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v9 15/29] x86/insn-eval: Add utility functions to get segment descriptor base address and limit
    From
    Date
    On Wed, 2017-10-11 at 17:15 +0200, Borislav Petkov wrote:
    > On Tue, Oct 03, 2017 at 08:54:18PM -0700, Ricardo Neri wrote:
    > >
    > > With segmentation, the base address of the segment is needed to compute a
    > > linear address. This base address is obtained from the applicable segment
    > > descriptor. Such segment descriptor is referenced from a segment selector.
    > ...
    >
    > >
    > > +unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
    > > +{
    > > + struct desc_struct *desc;
    > > + short sel;
    > > +
    > > + sel = get_segment_selector(regs, seg_reg_idx);
    > > + if (sel < 0)
    > > + return -1L;
    > > +
    > > + if (v8086_mode(regs))
    > > + /*
    > > +  * Base is simply the segment selector shifted 4
    > > +  * positions to the right.
    > > +  */
    > > + return (unsigned long)(sel << 4);
    > > +
    > > + if (user_64bit_mode(regs)) {
    > > + /*
    > > +  * Only FS or GS will have a base address, the rest of
    > > +  * the segments' bases are forced to 0.
    > > +  */
    > > + unsigned long base;
    > > +
    > > + if (seg_reg_idx == INAT_SEG_REG_FS)
    > > + rdmsrl(MSR_FS_BASE, base);
    > > + else if (seg_reg_idx == INAT_SEG_REG_GS)
    > > + /*
    > > +  * swapgs was called at the kernel entry point.
    > > Thus,
    > > +  * MSR_KERNEL_GS_BASE will have the user-space GS
    > > base.
    > > +  */
    > > + rdmsrl(MSR_KERNEL_GS_BASE, base);
    > > + else if (seg_reg_idx != INAT_SEG_REG_IGNORE)
    > > + /* We should ignore the rest of segment registers.
    > > */
    > > + base = -1L;
    > When is that case ever possible in long mode? You either have GS/FS
    > bases or 0. What's the meaning of -1L in that case?

    This is meant to be an error case. In long mode, only INAT_SEG_REG_IGNORE/FS/GS
    are valid. All other indices are invalid.

    Perhaps we could return -EINVAL instead?

    > Otherwise just minor things:
    >
    > ---
    > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
    > index 7ba5379a2923..e7e82b343bd0 100644
    > --- a/arch/x86/lib/insn-eval.c
    > +++ b/arch/x86/lib/insn-eval.c
    > @@ -515,13 +515,13 @@ static struct desc_struct *get_desc(unsigned short sel)
    >   *
    >   * Obtain the base address of the segment as indicated by the segment
    > descriptor
    >   * pointed by the segment selector. The segment selector is obtained from the
    > - * input segment register index seg_reg_idx.
    > + * input segment register index @seg_reg_idx.
    >   *
    >   * Returns:
    >   *
    >   * In protected mode, base address of the segment. Zero in long mode,
    >   * except when FS or GS are used. In virtual-8086 mode, the segment
    > - * selector shifted 4 positions to the right.
    > + * selector shifted 4 bits to the right.
    >   *
    >   * -1L in case of error.
    >   */
    > @@ -537,7 +537,7 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int
    > seg_reg_idx)
    >   if (v8086_mode(regs))
    >   /*
    >    * Base is simply the segment selector shifted 4
    > -  * positions to the right.
    > +  * bits to the right.
    >    */
    >   return (unsigned long)(sel << 4);

    Thanks! I will incorporate these changes along with your Improvements-by tag.

    BR,
    Ricardo

    \
     
     \ /
      Last update: 2017-10-11 21:58    [W:3.551 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site