[lkml]   [2015]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer

* Dave Hansen <> wrote:

> From: Dave Hansen <>
> 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:

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

- 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,

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

- 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

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

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).



 \ /
  Last update: 2015-05-28 11:21    [W:0.115 / U:11.292 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site