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

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

>
> From: Dave Hansen <dave.hansen@linux.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


\
 
 \ /
  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