Re: [PATCH v9 15/29] x86/insn-eval: Add utility functions to get segment descriptor base address and limit

Ricardo Neri
2017-10-11T19:57:44Z

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