Message-ID: <20150528084140.GA31719@gmail.com>
Date:	Thu, 28 May 2015 10:41:40 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Dave Hansen <dave@...1.net>
Cc:	linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
	dave.hansen@...ux.intel.com, oleg@...hat.com, bp@...en8.de,
	riel@...hat.com, sbsiddha@...il.com, luto@...capital.net,
	mingo@...hat.com, hpa@...or.com, fenghua.yu@...el.com
Subject: Re: [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer


* Dave Hansen <dave@...1.net> wrote:

> 
> From: Dave Hansen <dave.hansen@...ux.intel.com>
> 
> The MPX code appears to be saving off the FPU in an unsafe
> way.  It does not disable preemption or ensure that the
> FPU state has been allocated.  All of the preemption safety
> comes from the unfortunatley-named 'unlazy_fpu()'.

Btw., with the new FPU code these functions are named differently, and the bug in 
the MPX code became a lot more obvious:

     copy_fpregs_to_fpstate(&tsk->thread.fpu);
     xsave_buf = &(tsk->thread.fpu.state.xsave);
     bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);

it's indeed generally unsafe to access/copy FPU registers with preemption enabled, 
for two reasons:

  - on older systems that use FSAVE the instruction destroys FPU register
    contents, which has to be handled carefully

  - even on newer systems if we copy to FPU registers (which this code doesn't) 
    then we don't want a context switch to occur in the middle of it, because a 
    context switch will write to the fpstate, potentially overwriting our new data 
    with old FPU state.

But it's safe to access FPU registers with preemption enabled in a couple of 
special cases:

  - potentially destructively saving FPU registers: the signal handling code does
    this in copy_fpstate_to_sigframe(), because it can rely on the signal restore
    side to restore the original FPU state.

  - reading FPU registers on modern systems: we don't do this anywhere at the
    moment, mostly to keep symmetry with older systems where FSAVE is
    destructive.

  - initializing FPU registers on modern systems: fpu__clear() does this. Here
    it's safe because we don't copy from the fpstate.

  - directly writing FPU registers from user-space memory (!). We do this in
    fpu__restore_sig(), and it's safe because neither context switches nor
    irq-handler FPU use can corrupt the source context of the copy (which is
    user-space memory).

Note that the MPX code's current use of copy_fpregs_to_fpstate() was safe I think, 
because:

 - MPX is predicated on eagerfpu, so the destructive F[N]SAVE instruction won't be 
   used.

 - the code was only reading FPU registers, and was doing it only in places that
   guaranteed that an FPU state was already active (i.e. didn't do it in
   kthreads)

But ... I agree that a more robust API should be used to access FPU registers:

> @@ -427,3 +427,36 @@ void *get_xsave_addr(struct xregs_state
>  	return (void *)xsave + xstate_comp_offsets[feature_nr];
>  }
>  EXPORT_SYMBOL_GPL(get_xsave_addr);
>
> +/*
> + * This wraps up the common operations that need to occur when retrieving
> + * data from xsave state.  It first ensures that the current task was
> + * using the FPU and retrieves the data in to a buffer.  It then calculates
> + * the offset of the requested field in the buffer.
> + *
> + * This function is safe to call whether the FPU is in use or not.
> + *
> + * Note that this only works on the current task.
> + *
> + * Inputs:
> + *	@xsave_state: state which is defined in xsave.h (e.g. XSTATE_FP,
> + *	XSTATE_SSE, etc...)
> + * Output:
> + *	address of the state in the xsave area or NULL if the state
> + *	is not present or is in its 'init state'.
> + */
> +void *get_xsave_field_ptr(int xsave_state)

So this is retrieving (reading) data from FPU registers, but returns a writable 
'void *'. So the return pointer from this interface should be constified, to make 
sure no modifications may occur over them (which modificiations would be unsafe).

> +	union fpregs_state *xstate;
> +
> +	if (!current->thread.fpu.fpstate_active)
> +		return NULL;
> +	/*
> +	 * fpu__save() takes the CPU's xstate registers
> +	 * and saves them off to the 'fpu memory buffer.
> +	 */
> +	fpu__save(&current->thread.fpu);
> +	xstate = &current->thread.fpu.state;
> +
> +	return get_xsave_addr(&xstate->xsave, xsave_state);

Small nit, this would become a lot shorter if you introduced a helper local 
variable:

	struct fpu *fpu = &current->thread.fpu;

But more importantly, for a generic get_xsave_field_ptr() API, fpu__save() is not 
enough: fpu__save() will only save FPU registers into memory if necessary (i.e. if 
the FPU is already in use), and if you call it on a task with no FPU state then it 
will still have an !fpu->fpstate_active FPU state after the call, with random, 
invalid data in the xsave area.

What you want here is to make the (in-memory) FPU state valid and current, before 
reading it, and the function to use for that is fpu__activate_fpstate_read() 
(available in the latest tip:x86/fpu tree).

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists