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
