[patch 2/8] x86/fpu: Prevent state corruption in __fpu__restore_sig()

[patch 2/8] x86/fpu: Prevent state corruption in __fpu__restore_sig()

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

The non-compacted slowpath uses __copy_from_user() and copies the entire
user buffer into the kernel buffer, verbatim.  This means that the kernel
buffer may now contain entirely invalid state on which XRSTOR will #GP.
validate_user_xstate_header() can detect some of that corruption, but that
leaves the onus on callers to clear the buffer.

Prior to XSAVES support it was possible just to reinitialize the buffer,
completely, but with supervisor states that is not longer possible as the
buffer clearing code split got it backwards. Fixing that is possible, but
not corrupting the state in the first place is more robust.

Avoid corruption of the kernel XSAVE buffer by using copy_user_to_xstate()
which validates the XSAVE header contents before copying the actual states
to the kernel. copy_user_to_xstate() was previously only called for
compacted-format kernel buffers, but it works for both compacted and
non-compacted forms.

Using it for the non-compacted form is slower because of multiple
__copy_from_user() operations, but that cost is less important than robust
code in an already slow path.

[ Changelog polished by Dave Hansen ]

Fixes: b860eb8dce59 ("x86/fpu/xstate: Define new functions for clearing fpregs and xstates")
Reported-by: syzbot+2067e764dbcd10721e2e@syzkaller.appspotmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 arch/x86/include/asm/fpu/xstate.h |    4 ----
 arch/x86/kernel/fpu/signal.c      |    9 +--------
 arch/x86/kernel/fpu/xstate.c      |    2 +-
 3 files changed, 2 insertions(+), 13 deletions(-)

--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -112,8 +112,4 @@ void copy_supervisor_to_kernel(struct xr
 void copy_dynamic_supervisor_to_kernel(struct xregs_state *xstate, u64 mask);
 void copy_kernel_to_dynamic_supervisor(struct xregs_state *xstate, u64 mask);
 
-
-/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
-int validate_user_xstate_header(const struct xstate_header *hdr);
-
 #endif
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -405,14 +405,7 @@ static int __fpu__restore_sig(void __use
 	if (use_xsave() && !fx_only) {
 		u64 init_bv = xfeatures_mask_user() & ~user_xfeatures;
 
-		if (using_compacted_format()) {
-			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
-		} else {
-			ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
-
-			if (!ret && state_size > offsetof(struct xregs_state, header))
-				ret = validate_user_xstate_header(&fpu->state.xsave.header);
-		}
+		ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		if (ret)
 			goto err_out;
 
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -515,7 +515,7 @@ int using_compacted_format(void)
 }
 
 /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
-int validate_user_xstate_header(const struct xstate_header *hdr)
+static int validate_user_xstate_header(const struct xstate_header *hdr)
 {
 	/* No unknown or supervisor features may be set */
 	if (hdr->xfeatures & ~xfeatures_mask_user())

Re: [patch 2/8] x86/fpu: Prevent state corruption in __fpu__restore_sig()

[Borislav Petkov]

On Wed, Jun 02, 2021 at 11:55:45AM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The non-compacted slowpath uses __copy_from_user() and copies the entire
> user buffer into the kernel buffer, verbatim.  This means that the kernel
> buffer may now contain entirely invalid state on which XRSTOR will #GP.
> validate_user_xstate_header() can detect some of that corruption, but that
> leaves the onus on callers to clear the buffer.
> 
> Prior to XSAVES support it was possible just to reinitialize the buffer,
> completely, but with supervisor states that is not longer possible as the
> buffer clearing code split got it backwards. Fixing that is possible, but
> not corrupting the state in the first place is more robust.
> 
> Avoid corruption of the kernel XSAVE buffer by using copy_user_to_xstate()
> which validates the XSAVE header contents before copying the actual states
> to the kernel. copy_user_to_xstate() was previously only called for
> compacted-format kernel buffers, but it works for both compacted and
> non-compacted forms.
> 
> Using it for the non-compacted form is slower because of multiple
> __copy_from_user() operations, but that cost is less important than robust
> code in an already slow path.
> 
> [ Changelog polished by Dave Hansen ]

Nice polishing!

> Fixes: b860eb8dce59 ("x86/fpu/xstate: Define new functions for clearing fpregs and xstates")
> Reported-by: syzbot+2067e764dbcd10721e2e@syzkaller.appspotmail.com
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/include/asm/fpu/xstate.h |    4 ----
>  arch/x86/kernel/fpu/signal.c      |    9 +--------
>  arch/x86/kernel/fpu/xstate.c      |    2 +-
>  3 files changed, 2 insertions(+), 13 deletions(-)
> 
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -112,8 +112,4 @@ void copy_supervisor_to_kernel(struct xr
>  void copy_dynamic_supervisor_to_kernel(struct xregs_state *xstate, u64 mask);
>  void copy_kernel_to_dynamic_supervisor(struct xregs_state *xstate, u64 mask);
>  
> -
> -/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
> -int validate_user_xstate_header(const struct xstate_header *hdr);
> -
>  #endif
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -405,14 +405,7 @@ static int __fpu__restore_sig(void __use
>  	if (use_xsave() && !fx_only) {
>  		u64 init_bv = xfeatures_mask_user() & ~user_xfeatures;
>  
> -		if (using_compacted_format()) {
> -			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> -		} else {
> -			ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> -
> -			if (!ret && state_size > offsetof(struct xregs_state, header))
> -				ret = validate_user_xstate_header(&fpu->state.xsave.header);
> -		}
> +		ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
>  		if (ret)
>  			goto err_out;
>  
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -515,7 +515,7 @@ int using_compacted_format(void)
>  }
>  
>  /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
> -int validate_user_xstate_header(const struct xstate_header *hdr)
> +static int validate_user_xstate_header(const struct xstate_header *hdr)

Can't do that yet - that one is still called from regset.c:

arch/x86/kernel/fpu/regset.c: In function ‘xstateregs_set’:
arch/x86/kernel/fpu/regset.c:135:10: error: implicit declaration of function ‘validate_user_xstate_header’ [-Werror=implicit-function-declaration]
  135 |    ret = validate_user_xstate_header(&xsave->header);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

Maybe after the 5th patch which kills that usage too.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [patch 2/8] x86/fpu: Prevent state corruption in __fpu__restore_sig()

[Thomas Gleixner]

On Wed, Jun 02 2021 at 15:12, Borislav Petkov wrote:
>>  /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
>> -int validate_user_xstate_header(const struct xstate_header *hdr)
>> +static int validate_user_xstate_header(const struct xstate_header *hdr)
>
> Can't do that yet - that one is still called from regset.c:
>
> arch/x86/kernel/fpu/regset.c: In function ‘xstateregs_set’:
> arch/x86/kernel/fpu/regset.c:135:10: error: implicit declaration of function ‘validate_user_xstate_header’ [-Werror=implicit-function-declaration]
>   135 |    ret = validate_user_xstate_header(&xsave->header);
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
>
> Maybe after the 5th patch which kills that usage too.

Gah, yes. I had the patches ordered differently and then failed to do a
full step by step recompile after reshuffling them. Fixed localy.

Thanks,

        tglx

[patch V2 2/8] x86/fpu: Prevent state corruption in __fpu__restore_sig()

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

The non-compacted slowpath uses __copy_from_user() and copies the entire
user buffer into the kernel buffer, verbatim.  This means that the kernel
buffer may now contain entirely invalid state on which XRSTOR will #GP.
validate_user_xstate_header() can detect some of that corruption, but that
leaves the onus on callers to clear the buffer.

Prior to XSAVES support it was possible just to reinitialize the buffer,
completely, but with supervisor states that is not longer possible as the
buffer clearing code split got it backwards. Fixing that is possible, but
not corrupting the state in the first place is more robust.

Avoid corruption of the kernel XSAVE buffer by using copy_user_to_xstate()
which validates the XSAVE header contents before copying the actual states
to the kernel. copy_user_to_xstate() was previously only called for
compacted-format kernel buffers, but it works for both compacted and
non-compacted forms.

Using it for the non-compacted form is slower because of multiple
__copy_from_user() operations, but that cost is less important than robust
code in an already slow path.

[ Changelog polished by Dave Hansen ]

Fixes: b860eb8dce59 ("x86/fpu/xstate: Define new functions for clearing fpregs and xstates")
Reported-by: syzbot+2067e764dbcd10721e2e@syzkaller.appspotmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
V2: Removed the make validate_user_xstate_header() static hunks (Borislav)
---
 arch/x86/kernel/fpu/signal.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -405,14 +405,7 @@ static int __fpu__restore_sig(void __use
 	if (use_xsave() && !fx_only) {
 		u64 init_bv = xfeatures_mask_user() & ~user_xfeatures;
 
-		if (using_compacted_format()) {
-			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
-		} else {
-			ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
-
-			if (!ret && state_size > offsetof(struct xregs_state, header))
-				ret = validate_user_xstate_header(&fpu->state.xsave.header);
-		}
+		ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		if (ret)
 			goto err_out;
 

[patch 3/8] x86/fpu: Invalidate FPU state after a failed XRSTOR from a user buffer

[Thomas Gleixner]

From: Andy Lutomirski <luto@kernel.org>

If XRSTOR fails due to sufficiently complicated paging errors (e.g.
concurrent TLB invalidation), it may fault with #PF but still modify
portions of the user extended state.

If this happens in __fpu_restore_sig() with a victim task's FPU registers
loaded and the task is preempted by the victim task, the victim task
resumes with partially corrupt extended state.

Invalidate the FPU registers when XRSTOR fails to prevent this scenario.

Fixes: 1d731e731c4c ("x86/fpu: Add a fastpath to __fpu__restore_sig()")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/fpu/signal.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -369,6 +369,27 @@ static int __fpu__restore_sig(void __use
 			fpregs_unlock();
 			return 0;
 		}
+
+		if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
+			/*
+			 * The FPU registers do not belong to current, and
+			 * we just did an FPU restore operation, restricted
+			 * to the user portion of the register file, and
+			 * failed.  In the event that the ucode was
+			 * unfriendly and modified the registers at all, we
+			 * need to make sure that we aren't corrupting an
+			 * innocent non-current task's registers.
+			 */
+			__cpu_invalidate_fpregs_state();
+		} else {
+			/*
+			 * As above, we may have just clobbered current's
+			 * user FPU state.  We will either successfully
+			 * load it or clear it below, so no action is
+			 * required here.
+			 */
+		}
+
 		fpregs_unlock();
 	} else {
 		/*

Re: [patch 3/8] x86/fpu: Invalidate FPU state after a failed XRSTOR from a user buffer

[Borislav Petkov]

On Wed, Jun 02, 2021 at 11:55:46AM +0200, Thomas Gleixner wrote:
> From: Andy Lutomirski <luto@kernel.org>
> 
> If XRSTOR fails due to sufficiently complicated paging errors (e.g.
> concurrent TLB invalidation),

I can't connect "concurrent TLB invalidation" to "sufficiently
complicated paging errors". Can you elaborate pls?

> it may fault with #PF but still modify
> portions of the user extended state.

Yikes, leaky leaky insn.

> If this happens in __fpu_restore_sig() with a victim task's FPU registers
> loaded and the task is preempted by the victim task,

This is probably meaning another task but the only task mentioned here
is a "victim task"?

> the victim task
> resumes with partially corrupt extended state.
> 
> Invalidate the FPU registers when XRSTOR fails to prevent this scenario.
> 
> Fixes: 1d731e731c4c ("x86/fpu: Add a fastpath to __fpu__restore_sig()")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/kernel/fpu/signal.c |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -369,6 +369,27 @@ static int __fpu__restore_sig(void __use
>  			fpregs_unlock();
>  			return 0;
>  		}
> +
> +		if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
> +			/*
> +			 * The FPU registers do not belong to current, and
> +			 * we just did an FPU restore operation, restricted

Please get rid of the "we"-personal pronouns formulations.

> +			 * to the user portion of the register file, and

"register file"? That sounds like comment which belongs in microcode but
not in software. :-)

> +			 * failed.  In the event that the ucode was
> +			 * unfriendly and modified the registers at all, we
> +			 * need to make sure that we aren't corrupting an
> +			 * innocent non-current task's registers.
> +			 */
> +			__cpu_invalidate_fpregs_state();
> +		} else {
> +			/*
> +			 * As above, we may have just clobbered current's
> +			 * user FPU state.  We will either successfully
> +			 * load it or clear it below, so no action is
> +			 * required here.
> +			 */
> +		}

I'm wondering if that comment can simply be above the TIF_NEED_FPU_LOAD
testing, standalone, instead of having it in an empty else? And then get
rid of that else.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [patch 3/8] x86/fpu: Invalidate FPU state after a failed XRSTOR from a user buffer

[Andy Lutomirski]

On 6/2/21 8:06 AM, Borislav Petkov wrote:
> On Wed, Jun 02, 2021 at 11:55:46AM +0200, Thomas Gleixner wrote:
>> From: Andy Lutomirski <luto@kernel.org>
>>
>> If XRSTOR fails due to sufficiently complicated paging errors (e.g.
>> concurrent TLB invalidation),
> 
> I can't connect "concurrent TLB invalidation" to "sufficiently
> complicated paging errors". Can you elaborate pls?

Think "complex microarchitectural conditions".

How about:

As far as I can tell, both Intel and AMD consider it to be
architecturally valid for XRSTOR to fail with #PF but nonetheless change
user state.  The actual conditions under which this might occur are
unclear [1], but it seems plausible that this might be triggered if one
sibling thread unmaps a page and invalidates the shared TLB while
another sibling thread is executing XRSTOR on the page in question.

__fpu__restore_sig() can execute XRSTOR while the hardware registers are
preserved on behalf of a different victim task (using the
fpu_fpregs_owner_ctx mechanism), and, in theory, XRSTOR could fail but
modify the registers.  If this happens, then there is a window in which
__fpu__restore_sig() could schedule out and the victim task could
schedule back in without reloading its own FPU registers.  This would
result in part of the FPU state that __fpu__restore_sig() was attempting
to load leaking into the victim task's user-visible state.

Invalidate preserved FPU registers on XRSTOR failure to prevent this
situation from corrupting any state.

[1] Frequent readers of the errata lists might imagine "complex
microarchitectural conditions".

>> +			 * failed.  In the event that the ucode was
>> +			 * unfriendly and modified the registers at all, we
>> +			 * need to make sure that we aren't corrupting an
>> +			 * innocent non-current task's registers.
>> +			 */
>> +			__cpu_invalidate_fpregs_state();
>> +		} else {
>> +			/*
>> +			 * As above, we may have just clobbered current's
>> +			 * user FPU state.  We will either successfully
>> +			 * load it or clear it below, so no action is
>> +			 * required here.
>> +			 */
>> +		}
> 
> I'm wondering if that comment can simply be above the TIF_NEED_FPU_LOAD
> testing, standalone, instead of having it in an empty else? And then get
> rid of that else.

I'm fine either way.

Re: [patch 3/8] x86/fpu: Invalidate FPU state after a failed XRSTOR from a user buffer

[Borislav Petkov]

On Thu, Jun 03, 2021 at 10:30:05AM -0700, Andy Lutomirski wrote:
> Think "complex microarchitectural conditions".

Ah, the magic phrase.

> How about:
> 
> As far as I can tell, both Intel and AMD consider it to be
> architecturally valid for XRSTOR to fail with #PF but nonetheless change
> user state.  The actual conditions under which this might occur are
> unclear [1], but it seems plausible that this might be triggered if one
> sibling thread unmaps a page and invalidates the shared TLB while
> another sibling thread is executing XRSTOR on the page in question.
> 
> __fpu__restore_sig() can execute XRSTOR while the hardware registers are
> preserved on behalf of a different victim task (using the
> fpu_fpregs_owner_ctx mechanism), and, in theory, XRSTOR could fail but
> modify the registers.  If this happens, then there is a window in which
> __fpu__restore_sig() could schedule out and the victim task could
> schedule back in without reloading its own FPU registers.  This would
> result in part of the FPU state that __fpu__restore_sig() was attempting
> to load leaking into the victim task's user-visible state.
> 
> Invalidate preserved FPU registers on XRSTOR failure to prevent this
> situation from corrupting any state.
> 
> [1] Frequent readers of the errata lists might imagine "complex
> microarchitectural conditions".

Yap, very nice, thanks!

> > I'm wondering if that comment can simply be above the TIF_NEED_FPU_LOAD
> > testing, standalone, instead of having it in an empty else? And then get
> > rid of that else.
> 
> I'm fine either way.

Ok, then let's aim for common, no-surprise-there patterns as we're in a
mine field here anyway.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[patch 4/8] x86/fpu: Limit xstate copy size in xstateregs_set()

[Thomas Gleixner]

If the count argument is larger than the xstate size, this will happily
copy beyond the end of xstate.

Fixes: 91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/fpu/regset.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -117,7 +117,7 @@ int xstateregs_set(struct task_struct *t
 	/*
 	 * A whole standard-format XSAVE buffer is needed:
 	 */
-	if ((pos != 0) || (count < fpu_user_xstate_size))
+	if (pos != 0 || count != fpu_user_xstate_size)
 		return -EFAULT;
 
 	xsave = &fpu->state.xsave;

Re: [patch 4/8] x86/fpu: Limit xstate copy size in xstateregs_set()

[Borislav Petkov]

On Wed, Jun 02, 2021 at 11:55:47AM +0200, Thomas Gleixner wrote:
> If the count argument is larger than the xstate size, this will happily
> copy beyond the end of xstate.
> 
> Fixes: 91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/kernel/fpu/regset.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/arch/x86/kernel/fpu/regset.c
> +++ b/arch/x86/kernel/fpu/regset.c
> @@ -117,7 +117,7 @@ int xstateregs_set(struct task_struct *t
>  	/*
>  	 * A whole standard-format XSAVE buffer is needed:
>  	 */
> -	if ((pos != 0) || (count < fpu_user_xstate_size))
> +	if (pos != 0 || count != fpu_user_xstate_size)
>  		return -EFAULT;
>  
>  	xsave = &fpu->state.xsave;

Looking at this one, I guess it and all the CC:stable ones should be at
the beginning of the set so that they go to Linus now...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette