MXCSR is architecturally part of the SSE state. But, the kernel code
presumes it as part of the FP component. Adjust the offset and size for
these legacy states.

Notably, each legacy component area is not contiguous, unlike extended
components. Add a warning message when these offset and size are
referenced.

Fixes: ac73b27aea4e ("x86/fpu/xstate: Fix xstate_offsets, xstate_sizes for non-extended xstates")
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/fpu/xstate.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index a3f7045d1f8e..ac2ec5d6e7e4 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
 	 * offsets.
 	 */
 	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
-	    xfeature <= XFEATURE_SSE)
+	    xfeature <= XFEATURE_SSE) {
+		if (xfeature <= XFEATURE_SSE)
+			pr_warn("The legacy state (%d) is discontiguously located.\n",
+				xfeature);
+
 		return xstate_offsets[xfeature];
+	}
 
 	/*
 	 * Compacted format offsets depend on the actual content of the
@@ -217,14 +222,18 @@ static void __init setup_xstate_cache(void)
 	 * The FP xstates and SSE xstates are legacy states. They are always
 	 * in the fixed offsets in the xsave area in either compacted form
 	 * or standard form.
+	 *
+	 * But, while MXCSR is part of the SSE state, it is located in
+	 * between the FP states. Note that it is erroneous assuming that
+	 * each legacy area is contiguous.
 	 */
 	xstate_offsets[XFEATURE_FP]	= 0;
-	xstate_sizes[XFEATURE_FP]	= offsetof(struct fxregs_state,
-						   xmm_space);
+	xstate_sizes[XFEATURE_FP]	= offsetof(struct fxregs_state, mxcsr) +
+					  sizeof_field(struct fxregs_state, st_space);
 
-	xstate_offsets[XFEATURE_SSE]	= xstate_sizes[XFEATURE_FP];
-	xstate_sizes[XFEATURE_SSE]	= sizeof_field(struct fxregs_state,
-						       xmm_space);
+	xstate_offsets[XFEATURE_SSE]	= offsetof(struct fxregs_state, mxcsr);
+	xstate_sizes[XFEATURE_SSE]	= MXCSR_AND_FLAGS_SIZE +
+					  sizeof_field(struct fxregs_state, xmm_space);
 
 	for_each_extended_xfeature(i, fpu_kernel_cfg.max_features) {
 		cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
-- 
2.17.1

On Thu, Sep 22, 2022, Chang S. Bae wrote:
> MXCSR is architecturally part of the SSE state. But, the kernel code
> presumes it as part of the FP component. Adjust the offset and size for
> these legacy states.
> 
> Notably, each legacy component area is not contiguous, unlike extended
> components. Add a warning message when these offset and size are
> referenced.
> 
> Fixes: ac73b27aea4e ("x86/fpu/xstate: Fix xstate_offsets, xstate_sizes for non-extended xstates")
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  arch/x86/kernel/fpu/xstate.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index a3f7045d1f8e..ac2ec5d6e7e4 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
>  	 * offsets.
>  	 */
>  	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
> -	    xfeature <= XFEATURE_SSE)
> +	    xfeature <= XFEATURE_SSE) {
> +		if (xfeature <= XFEATURE_SSE)
> +			pr_warn("The legacy state (%d) is discontiguously located.\n",
> +				xfeature);

pr_warn() here isn't warranted.  copy_uabi_to_xstate() calls this with non-extended
features, which is perfectly fine since it manually handles MXCSR.  And that helper
is directly reachable by userspace, i.e. userspace can spam the pr_warn().

On 9/28/2022 2:06 PM, Sean Christopherson wrote:
> On Thu, Sep 22, 2022, Chang S. Bae wrote:
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index a3f7045d1f8e..ac2ec5d6e7e4 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
>>   	 * offsets.
>>   	 */
>>   	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
>> -	    xfeature <= XFEATURE_SSE)
>> +	    xfeature <= XFEATURE_SSE) {
>> +		if (xfeature <= XFEATURE_SSE)
>> +			pr_warn("The legacy state (%d) is discontiguously located.\n",
>> +				xfeature);
> 
> pr_warn() here isn't warranted.  copy_uabi_to_xstate() calls this with non-extended
> features, 

I think patch1 makes changes not to call this for legacy features anymore.

> which is perfectly fine since it manually handles MXCSR.  And that helper
> is directly reachable by userspace, i.e. userspace can spam the pr_warn().

I don't think I get your point. I assume that helper is 
__raw_xsave_addr(). But then I'm missing how it can be directly reached 
by userspace.

Thanks,
Chang

On Wed, Sep 28, 2022, Chang S. Bae wrote:
> On 9/28/2022 2:06 PM, Sean Christopherson wrote:
> > On Thu, Sep 22, 2022, Chang S. Bae wrote:
> > > 
> > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > > index a3f7045d1f8e..ac2ec5d6e7e4 100644
> > > --- a/arch/x86/kernel/fpu/xstate.c
> > > +++ b/arch/x86/kernel/fpu/xstate.c
> > > @@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
> > >   	 * offsets.
> > >   	 */
> > >   	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
> > > -	    xfeature <= XFEATURE_SSE)
> > > +	    xfeature <= XFEATURE_SSE) {
> > > +		if (xfeature <= XFEATURE_SSE)
> > > +			pr_warn("The legacy state (%d) is discontiguously located.\n",
> > > +				xfeature);
> > 
> > pr_warn() here isn't warranted.  copy_uabi_to_xstate() calls this with non-extended
> > features,
> 
> I think patch1 makes changes not to call this for legacy features anymore.

Oh, even better!  In that case, drop patch 3 and WARN here, because with that
call site gone, all paths that lead to xfeature_get_offset() avoid calling it
with legacy features.

The comment above this probably needs to be updated too.

I was going to make that suggestion in my original response, but I didn't apply
the series and so didn't see that that last remaining wart was fixed.  Thanks,
and sorry for the runaround!

> > which is perfectly fine since it manually handles MXCSR.  And that helper
> > is directly reachable by userspace, i.e. userspace can spam the pr_warn().
> 
> I don't think I get your point. I assume that helper is __raw_xsave_addr().
> But then I'm missing how it can be directly reached by userspace.

ioctl(KVM_GET_XSAVE) can reach this code, i.e. if there were a bug then userspace
could invoke that ioctl() in a tight loop to spam the kernel log.