Re: [PATCH 0/2] x86: UMIP emulation leaking kernel addresses

[Borislav Petkov]

On Wed, Dec 06, 2023 at 01:43:43AM +0100, Michal Luczaj wrote:
> Introducing a DPL check in insn_get_seg_base(), or even in get_desc(),
> seems enough to prevent the decoder from disclosing data.
> 
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 558a605929db..4c1eea736519 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -725,6 +725,18 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
>  	if (!get_desc(&desc, sel))
>  		return -1L;
>  
> +	/*
> +	 * Some segment selectors coming from @regs do not necessarily reflect
> +	 * the state of CPU; see get_segment_selector(). Their values might
> +	 * have been altered by ptrace. Thus, the instruction decoder can be
> +	 * tricked into "dereferencing" a segment descriptor that would
> +	 * otherwise cause a CPU exception -- for example due to mismatched
> +	 * privilege levels. This opens up the possibility to expose kernel
> +	 * space base address of DPL=0 segments.
> +	 */
> +	if (desc.dpl < (regs->cs & SEGMENT_RPL_MASK))
> +		return -1L;
> +
>  	return get_desc_base(&desc);
>  }
>  
> That said, I guess instead of trying to harden the decoder,

Well, here's what my CPU manual says:

"4.10.1 Accessing Data Segments

...

The processor compares the effective privilege level with the DPL in the
descriptor-table entry referenced by the segment selector. If the
effective privilege level is greater than or equal to (numerically
lower-than or equal-to) the DPL, then the processor loads the segment
register with the data-segment selector. 

If the effective privilege level is lower than (numerically
greater-than) the DPL, a general-protection exception (#GP) occurs and
the segment register is not loaded.

...

4.10.2 Accessing Stack Segments

The processor compares the CPL with the DPL in the descriptor-table
entry referenced by the segment selector. The two values must be equal.
If they are not equal, a #GP occurs and the SS register is not loaded."

So *actually* doing those checks in the insn decoder is the proper thing
to do, IMNSVHO.

> Now, I'm far from being competent, but here's an idea I've tried: tell
> the #GP handler that UMIP-related exceptions come only as #GP(0):
> 
>  	if (static_cpu_has(X86_FEATURE_UMIP)) {
> -		if (user_mode(regs) && fixup_umip_exception(regs))
> +		if (user_mode(regs) && !error_code && fixup_umip_exception(regs))
>  			goto exit;
>  	}

And yap, as you've realized, that alone doesn't fix the leaking.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette