Re: [PATCH 3/4] x86/fpu: Add consistency check between xstate_size and xfeatures

[Ingo Molnar]

* Andrei Vagin <avagin@xxxxxxxxxx> wrote:

> The signal frame is designed to be self-describing, where xstate_size
> indicates the actual size of the xstate context. The kernel previously
> lacked a check to ensure that the provided xstate_size was sufficient
> for the features enabled in the xfeatures mask. Additionally,
> restore_fpregs_from_user() always used the default xstate_size to fault
> in the xstate user buffer.
> 
> These consistency checks have been added:
> * Validate that xfeatures is a subset of the features enabled for the
>   task.
> * Calculate the required size for the validated xfeatures mask.
> * Ensure the provided xstate_size is sufficient.
> 
> These checks prevent the kernel from attempting to fault in memory past
> the end of a frame.
> 
> Signed-off-by: Andrei Vagin <avagin@xxxxxxxxxx>
> ---
>  arch/x86/kernel/fpu/signal.c | 29 +++++++++++++++++++++++------
>  arch/x86/kernel/fpu/xstate.c |  2 +-
>  arch/x86/kernel/fpu/xstate.h |  2 ++
>  3 files changed, 26 insertions(+), 7 deletions(-)

So since this is a potentially invasive change, could you please split
it up into further incremental steps, with the behavioral changes at
the end:

  x86/fpu: Export xstate_calculate_size() internally
  x86/fpu: Extend restore_fpregs_from_user() with 'xstate_size'
  x86/fpu: Rename 'fpstate' to 'sig_fpstate' in check_xstate_in_sigframe()
  x86/fpu: Introduce 'fpstate' helper variable in check_xstate_in_sigframe()
  etc.

To make it all more reviewable & bisectable.

Also, could the check_xstate_in_sigframe() function get some TLC before
we modify it materially:

  1)

  arch/x86/kernel/fpu/signal.c:static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
  arch/x86/kernel/fpu/signal.c:           if (!check_xstate_in_sigframe(buf_fx, &fx_sw_user))

  Please harmonize the argument names: why is it 'buf_fx' in one
  function and 'fxbuf' in another? We should probably standardize
  on 'buf_fx' everywhere.

  2)

  Why is an error condition label called 'setfx'? How about
  'error_setfx' or so. Makes patches more straightforward to read.

etc., I'm sure there's more.

Plus regarding the behavioral changes, this one should probably
be its own patch as well:

+       /* Enforce XFEATURE_MASK_FPSSE when XSAVE is enabled */
+       fx_sw->xfeatures |= XFEATURE_MASK_FPSSE;

With the new size checks in another patch, right?

Thanks,

	Ingo