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

[Ricardo Neri]

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