[v7,38/41] x86/fpu: Add helper for initing features

  If an xfeature is saved in a buffer, the xfeature's bit will be set in
xsave->header.xfeatures. The CPU may opt to not save the xfeature if it
is in it's init state. In this case the xfeature buffer address cannot
be retrieved with get_xsave_addr().

Future patches will need to handle the case of writing to an xfeature
that may not be saved. So provide helpers to init an xfeature in an
xsave buffer.

This could of course be done directly by reaching into the xsave buffer,
however this would not be robust against future changes to optimize the
xsave buffer by compacting it. In that case the xsave buffer would need
to be re-arranged as well. So the logic properly belongs encapsulated
in a helper where the logic can be unified.

Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Tested-by: Kees Cook <keescook@chromium.org>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

---
v2:
 - New patch
---
 arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++++++++++-------
 arch/x86/kernel/fpu/xstate.h |  6 ++++
 2 files changed, 53 insertions(+), 11 deletions(-)
  

On Mon, Feb 27, 2023 at 02:29:54PM -0800, Rick Edgecombe wrote:
> Subject: Re: [PATCH v7 38/41] x86/fpu: Add helper for initing features

"initializing"

> If an xfeature is saved in a buffer, the xfeature's bit will be set in
> xsave->header.xfeatures. The CPU may opt to not save the xfeature if it
> is in it's init state. In this case the xfeature buffer address cannot

"its"

> be retrieved with get_xsave_addr().
> 
> Future patches will need to handle the case of writing to an xfeature
> that may not be saved. So provide helpers to init an xfeature in an
> xsave buffer.
> 
> This could of course be done directly by reaching into the xsave buffer,
> however this would not be robust against future changes to optimize the
> xsave buffer by compacting it. In that case the xsave buffer would need
> to be re-arranged as well. So the logic properly belongs encapsulated
> in a helper where the logic can be unified.
> 
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> Tested-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> ---
> v2:
>  - New patch
> ---
>  arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++++++++++-------
>  arch/x86/kernel/fpu/xstate.h |  6 ++++
>  2 files changed, 53 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 13a80521dd51..3ff80be0a441 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -934,6 +934,24 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>  	return (void *)xsave + xfeature_get_offset(xcomp_bv, xfeature_nr);
>  }
>  
> +static int xsave_buffer_access_checks(int xfeature_nr)

Function name needs a verb.

> +{
> +	/*
> +	 * Do we even *have* xsave state?
> +	 */

That comment is superfluous.

> +	if (!boot_cpu_has(X86_FEATURE_XSAVE))

check_for_deprecated_apis: WARNING: arch/x86/kernel/fpu/xstate.c:942: Do not use boot_cpu_has() - use cpu_feature_enabled() instead.

> +		return 1;
> +
> +	/*
> +	 * We should not ever be requesting features that we

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

> +	 * have not enabled.
> +	 */
> +	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> +		return 1;
> +
> +	return 0;
> +}
> +
>  /*
>   * Given the xsave area and a state inside, this function returns the
>   * address of the state.
> @@ -954,17 +972,7 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>   */
>  void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>  {
> -	/*
> -	 * Do we even *have* xsave state?
> -	 */
> -	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> -		return NULL;
> -
> -	/*
> -	 * We should not ever be requesting features that we
> -	 * have not enabled.
> -	 */
> -	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> +	if (xsave_buffer_access_checks(xfeature_nr))
>  		return NULL;
>  
>  	/*
> @@ -984,6 +992,34 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>  	return __raw_xsave_addr(xsave, xfeature_nr);
>  }
>  
> +/*
> + * Given the xsave area and a state inside, this function
> + * initializes an xfeature in the buffer.

s/this function initializes/initialize/

> + *
> + * get_xsave_addr() will return NULL if the feature bit is
> + * not present in the header. This function will make it so
> + * the xfeature buffer address is ready to be retrieved by
> + * get_xsave_addr().

So users of get_xsave_addr() would have to know that they would need to
call init_xfeature()?

I think the better approach would be:

void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr, bool init)

and then that @init controls whether get_xsave_addr() should init the
buffer.

And then you don't have to have a bunch of small functions here and
there and know when to call what but get_xsave_addr() would simply DTRT.

Thx.
  

On Sat, 2023-03-11 at 13:54 +0100, Borislav Petkov wrote:
> On Mon, Feb 27, 2023 at 02:29:54PM -0800, Rick Edgecombe wrote:
> > Subject: Re: [PATCH v7 38/41] x86/fpu: Add helper for initing
> > features
> 
> "initializing"

Sure.

> 
> > If an xfeature is saved in a buffer, the xfeature's bit will be set
> > in
> > xsave->header.xfeatures. The CPU may opt to not save the xfeature
> > if it
> > is in it's init state. In this case the xfeature buffer address
> > cannot
> 
> "its"

I clearly need to be better about it's and its.

> 
> > be retrieved with get_xsave_addr().
> > 
> > Future patches will need to handle the case of writing to an
> > xfeature
> > that may not be saved. So provide helpers to init an xfeature in an
> > xsave buffer.
> > 
> > This could of course be done directly by reaching into the xsave
> > buffer,
> > however this would not be robust against future changes to optimize
> > the
> > xsave buffer by compacting it. In that case the xsave buffer would
> > need
> > to be re-arranged as well. So the logic properly belongs
> > encapsulated
> > in a helper where the logic can be unified.
> > 
> > Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> > Tested-by: John Allen <john.allen@amd.com>
> > Tested-by: Kees Cook <keescook@chromium.org>
> > Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > 
> > ---
> > v2:
> >  - New patch
> > ---
> >  arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++++++++++---
> > ----
> >  arch/x86/kernel/fpu/xstate.h |  6 ++++
> >  2 files changed, 53 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/fpu/xstate.c
> > b/arch/x86/kernel/fpu/xstate.c
> > index 13a80521dd51..3ff80be0a441 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -934,6 +934,24 @@ static void *__raw_xsave_addr(struct
> > xregs_state *xsave, int xfeature_nr)
> >  	return (void *)xsave + xfeature_get_offset(xcomp_bv,
> > xfeature_nr);
> >  }
> >  
> > +static int xsave_buffer_access_checks(int xfeature_nr)
> 
> Function name needs a verb.

Right.

> 
> > +{
> > +	/*
> > +	 * Do we even *have* xsave state?
> > +	 */
> 
> That comment is superfluous.
> 
> > +	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> 
> check_for_deprecated_apis: WARNING: arch/x86/kernel/fpu/xstate.c:942:
> Do not use boot_cpu_has() - use cpu_feature_enabled() instead.
> 
> > +		return 1;
> > +
> > +	/*
> > +	 * We should not ever be requesting features that we
> 
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.

These two are from the existing code. Basically they get extracted into
a new function.

> 
> > +	 * have not enabled.
> > +	 */
> > +	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Given the xsave area and a state inside, this function returns
> > the
> >   * address of the state.
> > @@ -954,17 +972,7 @@ static void *__raw_xsave_addr(struct
> > xregs_state *xsave, int xfeature_nr)
> >   */
> >  void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
> >  {
> > -	/*
> > -	 * Do we even *have* xsave state?
> > -	 */
> > -	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> > -		return NULL;
> > -
> > -	/*
> > -	 * We should not ever be requesting features that we
> > -	 * have not enabled.
> > -	 */
> > -	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> > +	if (xsave_buffer_access_checks(xfeature_nr))
> >  		return NULL;
> >  
> >  	/*
> > @@ -984,6 +992,34 @@ void *get_xsave_addr(struct xregs_state
> > *xsave, int xfeature_nr)
> >  	return __raw_xsave_addr(xsave, xfeature_nr);
> >  }
> >  
> > +/*
> > + * Given the xsave area and a state inside, this function
> > + * initializes an xfeature in the buffer.
> 
> s/this function initializes/initialize/

Sure.

> 
> > + *
> > + * get_xsave_addr() will return NULL if the feature bit is
> > + * not present in the header. This function will make it so
> > + * the xfeature buffer address is ready to be retrieved by
> > + * get_xsave_addr().
> 
> So users of get_xsave_addr() would have to know that they would need
> to
> call init_xfeature()?

That is the situation today. FWIW both of these functions are limited
to the FPU internals, so I would think it's not a too unreasonable
assumption.

> 
> I think the better approach would be:
> 
> void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr, bool
> init)
> 
> and then that @init controls whether get_xsave_addr() should init the
> buffer.
> 
> And then you don't have to have a bunch of small functions here and
> there and know when to call what but get_xsave_addr() would simply
> DTRT.

It would have to actually copy the init state to the buffer
from init_fpstate, because otherwise the caller couldn't know 
if get_xsave_addr() was returning valid data or some old data in the
buffer. And I guess the `init` mean to initialize it only if it is in
the init state, not to overwrite the current state with the init state.

I did it up, and it makes the caller code cleaner. But I'm not sure
what to think of it. Is this not mixing two operations together? Today
get_xsave_addr() pretty much just gets a buffer offset with some
checks. Now it would compute the offset and also silently go off and
changes the buffer.

I looked at this fpu code originally and thought I could add some
useful abstractions, but this failed. I came away wondering if this was
just an area with so many special cases and details, that abstractions
just added confusion. I'm just bringing this up because the other
option is to just do this in the regset code:
xsave->header.xfeatures |= BIT_ULL(XFEATURE_CET_USER);

Let me know if you think it would be better to just open code it.

> 
> Thx.
>
  

On Mon, Mar 13, 2023 at 02:45:08AM +0000, Edgecombe, Rick P wrote:
> These two are from the existing code. Basically they get extracted into
> a new function.

I know but you can fix them while at it.

> I did it up, and it makes the caller code cleaner. But I'm not sure
> what to think of it. Is this not mixing two operations together? Today
> get_xsave_addr() pretty much just gets a buffer offset with some
> checks. Now it would compute the offset and also silently go off and
> changes the buffer.

Ok, so why don't you write the call site this way instead:

        cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER);
        if (!cetregs) {
		if (xfeature_saved(xsave, XFEATURE_CET_USER)) {
			WARN("something's wrong with this buffer")
			return ...;
		}

		/* Not saved, initialize it */
		init_xfeature(xsave, XFEATURE_CET_USER));
	}

	cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER);
	if (!cetregs) {
		WARN_ON("WTF")
                return -ENODEV;
	}

Now it is clear what happens and it is a common code pattern of trying
to get something and initializing it if it wasn't initialized yet, and
then retrying...

Hmm?
  

On Mon, 2023-03-13 at 12:03 +0100, Borislav Petkov wrote:
> On Mon, Mar 13, 2023 at 02:45:08AM +0000, Edgecombe, Rick P wrote:
> > These two are from the existing code. Basically they get extracted
> > into
> > a new function.
> 
> I know but you can fix them while at it.

Ok.

> 
> > I did it up, and it makes the caller code cleaner. But I'm not sure
> > what to think of it. Is this not mixing two operations together?
> > Today
> > get_xsave_addr() pretty much just gets a buffer offset with some
> > checks. Now it would compute the offset and also silently go off
> > and
> > changes the buffer.
> 
> Ok, so why don't you write the call site this way instead:
> 
>         cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER);
>         if (!cetregs) {
>                 if (xfeature_saved(xsave, XFEATURE_CET_USER)) {
>                         WARN("something's wrong with this buffer")
>                         return ...;
>                 }
> 
>                 /* Not saved, initialize it */
>                 init_xfeature(xsave, XFEATURE_CET_USER));
>         }
> 
>         cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER);
>         if (!cetregs) {
>                 WARN_ON("WTF")
>                 return -ENODEV;
>         }
> 
> Now it is clear what happens and it is a common code pattern of
> trying
> to get something and initializing it if it wasn't initialized yet,
> and
> then retrying...
> 
> Hmm?

This seems more clear. I'm sorry for the noise here though, because
this has made me realize that the initing logic should never be hit. We
used to support the full CET_U state in ptrace, but then dropped it to
just the SSP and only allowed it when shadow stack is active. This
means that CET_U will always have at least the CET_SHSTK_EN bit set and
so not be in the init state. So this can probably just warn and bail if
it sees an init state.

Unless the extra logic seems more robust? But it is always nice when
the chance comes to drop a patch out of this thing...
  

On Mon, Mar 13, 2023 at 04:10:14PM +0000, Edgecombe, Rick P wrote:
> This seems more clear. I'm sorry for the noise here though, because
> this has made me realize that the initing logic should never be hit. We
> used to support the full CET_U state in ptrace, but then dropped it to
> just the SSP and only allowed it when shadow stack is active.

Right, you do check that at function entry.

> This means that CET_U will always have at least the CET_SHSTK_EN bit
> set and so not be in the init state. So this can probably just warn
> and bail if it sees an init state.

I don't mind the additional checks as this is a security thing so
sanity checks are good, especially if they're cheap.

And you don't need to reinit the buffer - just scream loudly when get_xsave_addr()
returns NULL.
  

On Mon, 2023-03-13 at 18:10 +0100, Borislav Petkov wrote:
> > This means that CET_U will always have at least the CET_SHSTK_EN
> > bit
> > set and so not be in the init state. So this can probably just warn
> > and bail if it sees an init state.
> 
> I don't mind the additional checks as this is a security thing so
> sanity checks are good, especially if they're cheap.
> 
> And you don't need to reinit the buffer - just scream loudly when
> get_xsave_addr()
> returns NULL.

Ok, will do this instead in "x86: Add PTRACE interface for shadow
stack".