[PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[Chang S. Bae]

Changes from V3:
* Rebased onto 5.17-rc1.

V3: https://lore.kernel.org/lkml/20211110003209.21666-1-chang.seok.bae@intel.com/

---

The recent x86 dynamic state support incorporates the arch_prctl option to
request permission before using a dynamic state.

It was designed to add the requested feature in the group leader's
permission bitmask so that every thread can reference this master bitmask.
The group leader is assumed to be unchanged here. The mainline is the case
as a group leader is identified at fork() or exec() time only.

This master bitmask should include non-dynamic features always, as they
are permitted by default. Users may check them via ARCH_GET_XCOMP_PERM.

But, in hindsight, the implementation does overwrite the bitmask with the
requested bit only, instead of adding the bit to the existing one. This
overwrite effectively revokes the permission that is granted already.

Fix the code and also update the selftest to disclose the issue if there
is.

Chang S. Bae (1):
  selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test

Yang Zhong (1):
  x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation

 arch/x86/kernel/fpu/xstate.c      |  2 +-
 tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)


base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
-- 
2.17.1

[PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation

[Chang S. Bae]

From: Yang Zhong <yang.zhong@intel.com>

ARCH_REQ_XCOMP_PERM is supposed to add the requested feature to the
permission bitmap of thread_group_leader()->fpu. But the code overwrites
the bitmap with the requested feature bit only rather than adding it.

Fix the code to add the request feature bit to the master bitmask.

Fixes: db8268df0983 ("x86/arch_prctl: Add controls for dynamic XSTATE components")
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from v3:
* Adjust the change to the mainline (v5.17-rc).

Changes from v2:
 * Fix the authorship.

Changes from v1:
 * Change the mask value only and trim the changelog.
---
 arch/x86/kernel/fpu/xstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 02b3ddaf4f75..2d4363e32517 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
 
 	perm = guest ? &fpu->guest_perm : &fpu->perm;
 	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
-	WRITE_ONCE(perm->__state_perm, requested);
+	WRITE_ONCE(perm->__state_perm, mask);
 	/* Protected by sighand lock */
 	perm->__state_size = ksize;
 	perm->__user_state_size = usize;
-- 
2.17.1

Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation

[Hao Xiang]

x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation

If WRITE_ONCE(perm->__state_perm, requested) is modified to
WRITE_ONCE(perm->__state_perm, mask), When the qemu process does not 
request the XFEATURE_MASK_XTILE_DATA xsave state permission, there may 
be a gp error (kvm: kvm_set_xcr line 1091 inject gp fault with cpl 0) 
because __kvm_set_xcr return 1.

static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr){
     ...
     // xcr0 includes XFEATURE_MASK_XTILE_CFG by default.
     if ((xcr0 & XFEATURE_MASK_XTILE) &&
         ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE))
         return 1;
     ...
}

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 02b3dda..2d4363e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted, 
u64 requested, bool guest)

         perm = guest ? &fpu->guest_perm : &fpu->perm;
         /* Pairs with the READ_ONCE() in xstate_get_group_perm() */
-       WRITE_ONCE(perm->__state_perm, requested);
+       WRITE_ONCE(perm->__state_perm, mask);
         /* Protected by sighand lock */
         perm->__state_size = ksize;
         perm->__user_state_size = usize;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 494d4d3..e8704568 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -908,6 +908,9 @@ static inline int __do_cpuid_func(struct 
kvm_cpuid_array *array, u32 function)
                 break;
         case 0xd: {
                 u64 permitted_xcr0 = supported_xcr0 & 
xstate_get_guest_group_perm();
+               permitted_xcr0 = ((permitted_xcr0 & 
XFEATURES_MASK_XTILE) != XFEATURES_MASK_XTILE)
+                               ? permitted_xcr0
+                               : permitted_xcr0 & ~XFEATURES_MASK_XTILE;
                 u64 permitted_xss = supported_xss;

                 entry->eax &= permitted_xcr0;

Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation

[Chang S. Bae]

On 3/7/2022 4:20 AM, Hao Xiang wrote:
> x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
> 
> If WRITE_ONCE(perm->__state_perm, requested) is modified to
> WRITE_ONCE(perm->__state_perm, mask), When the qemu process does not 
> request the XFEATURE_MASK_XTILE_DATA xsave state permission, there may 
> be a gp error (kvm: kvm_set_xcr line 1091 inject gp fault with cpl 0) 
> because __kvm_set_xcr return 1.

What you said here does not make sense to me. When the Qemu process does 
not request XTILEDATA, then the __xstate_request_perm() function is 
never called in this, no?

> 
> static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr){
>      ...
>      // xcr0 includes XFEATURE_MASK_XTILE_CFG by default.
>      if ((xcr0 & XFEATURE_MASK_XTILE) &&
>          ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE))
>          return 1;
>      ...
> }
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 02b3dda..2d4363e 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted, 
> u64 requested, bool guest)
> 
>          perm = guest ? &fpu->guest_perm : &fpu->perm;
>          /* Pairs with the READ_ONCE() in xstate_get_group_perm() */
> -       WRITE_ONCE(perm->__state_perm, requested);
> +       WRITE_ONCE(perm->__state_perm, mask);
>          /* Protected by sighand lock */
>          perm->__state_size = ksize;
>          perm->__user_state_size = usize;
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 494d4d3..e8704568 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -908,6 +908,9 @@ static inline int __do_cpuid_func(struct 
> kvm_cpuid_array *array, u32 function)
>                  break;
>          case 0xd: {
>                  u64 permitted_xcr0 = supported_xcr0 & 
> xstate_get_guest_group_perm();

Yang, I think you should have included your fix [1] in your series [2] 
in the first place, before using it widely like [3].

> +               permitted_xcr0 = ((permitted_xcr0 & 
> XFEATURES_MASK_XTILE) != XFEATURES_MASK_XTILE)
> +                               ? permitted_xcr0
> +                               : permitted_xcr0 & ~XFEATURES_MASK_XTILE;
>                  u64 permitted_xss = supported_xss;
> 
>                  entry->eax &= permitted_xcr0;
> 

Well, first of all, one patch should fix one issue, not two or more, no?

But this hunk looks duplicate with this [4].

Thanks,
Chang


[1] https://lore.kernel.org/lkml/20211108222815.4078-1-yang.zhong@intel.com/
[2] 
https://lore.kernel.org/lkml/20220105123532.12586-1-yang.zhong@intel.com/
[3] 
https://lore.kernel.org/lkml/20220105123532.12586-2-yang.zhong@intel.com/
[4] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/x86.c#n1033

Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation

[Hao Xiang]

Using the linux upsteam code with your patches，the problem is not 
reproduced. Maybe my patches are incomplete.

Thanks,
Hao

On 2022/3/8 2:53, Chang S. Bae wrote:
> On 3/7/2022 4:20 AM, Hao Xiang wrote:
>> x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
>>
>> If WRITE_ONCE(perm->__state_perm, requested) is modified to
>> WRITE_ONCE(perm->__state_perm, mask), When the qemu process does not 
>> request the XFEATURE_MASK_XTILE_DATA xsave state permission, there may 
>> be a gp error (kvm: kvm_set_xcr line 1091 inject gp fault with cpl 0) 
>> because __kvm_set_xcr return 1.
> 
> What you said here does not make sense to me. When the Qemu process does 
> not request XTILEDATA, then the __xstate_request_perm() function is 
> never called in this, no?
> 
>>
>> static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr){
>>      ...
>>      // xcr0 includes XFEATURE_MASK_XTILE_CFG by default.
>>      if ((xcr0 & XFEATURE_MASK_XTILE) &&
>>          ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE))
>>          return 1;
>>      ...
>> }
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index 02b3dda..2d4363e 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted, 
>> u64 requested, bool guest)
>>
>>          perm = guest ? &fpu->guest_perm : &fpu->perm;
>>          /* Pairs with the READ_ONCE() in xstate_get_group_perm() */
>> -       WRITE_ONCE(perm->__state_perm, requested);
>> +       WRITE_ONCE(perm->__state_perm, mask);
>>          /* Protected by sighand lock */
>>          perm->__state_size = ksize;
>>          perm->__user_state_size = usize;
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 494d4d3..e8704568 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -908,6 +908,9 @@ static inline int __do_cpuid_func(struct 
>> kvm_cpuid_array *array, u32 function)
>>                  break;
>>          case 0xd: {
>>                  u64 permitted_xcr0 = supported_xcr0 & 
>> xstate_get_guest_group_perm();
> 
> Yang, I think you should have included your fix [1] in your series [2] 
> in the first place, before using it widely like [3].
> 
>> +               permitted_xcr0 = ((permitted_xcr0 & 
>> XFEATURES_MASK_XTILE) != XFEATURES_MASK_XTILE)
>> +                               ? permitted_xcr0
>> +                               : permitted_xcr0 & ~XFEATURES_MASK_XTILE;
>>                  u64 permitted_xss = supported_xss;
>>
>>                  entry->eax &= permitted_xcr0;
>>
> 
> Well, first of all, one patch should fix one issue, not two or more, no?
> 
> But this hunk looks duplicate with this [4].
> 
> Thanks,
> Chang
> 
> 
> [1] 
> https://lore.kernel.org/lkml/20211108222815.4078-1-yang.zhong@intel.com/
> [2] 
> https://lore.kernel.org/lkml/20220105123532.12586-1-yang.zhong@intel.com/
> [3] 
> https://lore.kernel.org/lkml/20220105123532.12586-2-yang.zhong@intel.com/
> [4] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/x86.c#n1033 
> 

[tip: x86/urgent] x86/fpu/xstate: Fix the ARCH_REQ_XCOMP_PERM implementation

[tip-bot2 for Yang Zhong]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     063452fd94d153d4eb38ad58f210f3d37a09cca4
Gitweb:        https://git.kernel.org/tip/063452fd94d153d4eb38ad58f210f3d37a09cca4
Author:        Yang Zhong <yang.zhong@intel.com>
AuthorDate:    Sat, 29 Jan 2022 09:36:46 -08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 23 Mar 2022 21:28:34 +01:00

x86/fpu/xstate: Fix the ARCH_REQ_XCOMP_PERM implementation

ARCH_REQ_XCOMP_PERM is supposed to add the requested feature to the
permission bitmap of thread_group_leader()->fpu. But the code overwrites
the bitmap with the requested feature bit only rather than adding it.

Fix the code to add the requested feature bit to the master bitmask.

Fixes: db8268df0983 ("x86/arch_prctl: Add controls for dynamic XSTATE components")
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Paolo Bonzini <bonzini@gnu.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20220129173647.27981-2-chang.seok.bae@intel.com

---
 arch/x86/kernel/fpu/xstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 7c7824a..dc6d5e9 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1639,7 +1639,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
 
 	perm = guest ? &fpu->guest_perm : &fpu->perm;
 	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
-	WRITE_ONCE(perm->__state_perm, requested);
+	WRITE_ONCE(perm->__state_perm, mask);
 	/* Protected by sighand lock */
 	perm->__state_size = ksize;
 	perm->__user_state_size = usize;

[PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test

[Chang S. Bae]

Update the arch_prctl test to check the permission bitmap whether the
requested feature is added as expected or not.

Every non-dynamic feature that is enabled is permitted already for use.
TILECFG is not dynamic feature. Ensure the bit is always on from
ARCH_GET_XCOMP_PERM.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-kselftest@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c
index 3615ef4a48bb..e1e2c8f3356f 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -368,9 +368,16 @@ static void req_xtiledata_perm(void)
 
 static void validate_req_xcomp_perm(enum expected_result exp)
 {
-	unsigned long bitmask;
+	unsigned long bitmask, expected_bitmask;
 	long rc;
 
+	rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
+	if (rc) {
+		fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
+	} else if (!(bitmask & XFEATURE_MASK_XTILECFG)) {
+		fatal_error("ARCH_GET_XCOMP_PERM returns XFEATURE_XTILECFG off.");
+	}
+
 	rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_PERM, XFEATURE_XTILEDATA);
 	if (exp == FAIL_EXPECTED) {
 		if (rc) {
@@ -383,10 +390,15 @@ static void validate_req_xcomp_perm(enum expected_result exp)
 		fatal_error("ARCH_REQ_XCOMP_PERM saw unexpected failure.\n");
 	}
 
+	expected_bitmask = bitmask | XFEATURE_MASK_XTILEDATA;
+
 	rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
 	if (rc) {
 		fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
-	} else if (bitmask & XFEATURE_MASK_XTILE) {
+	} else if (bitmask != expected_bitmask) {
+		fatal_error("ARCH_REQ_XCOMP_PERM saw a wrong bitmask: %lx, expected: %lx.\n",
+			    bitmask, expected_bitmask);
+	} else {
 		printf("\tARCH_REQ_XCOMP_PERM is successful.\n");
 	}
 }
-- 
2.17.1

Re: [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test

[Thomas Gleixner]

On Sat, Jan 29 2022 at 09:36, Chang S. Bae wrote:
> Update the arch_prctl test to check the permission bitmap whether the
> requested feature is added as expected or not.
>
> Every non-dynamic feature that is enabled is permitted already for use.
> TILECFG is not dynamic feature. Ensure the bit is always on from
> ARCH_GET_XCOMP_PERM.

Running it on a machine which does not have AMX results in:

 amx_64: [FAIL]	xstate cpuid: invalid tile data size/offset: 0/0: Success

It's not a failure, really. Selftests are supposed to run on all
machines and the proper thing to do if a hardware feature is not
available is to SKIP the test and return 0.

Thanks,

        tglx

Re: [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test

[Chang S. Bae]

On 3/23/2022 9:44 AM, Thomas Gleixner wrote:
> 
> Running it on a machine which does not have AMX results in:
> 
>   amx_64: [FAIL]	xstate cpuid: invalid tile data size/offset: 0/0: Success
> 
> It's not a failure, really. Selftests are supposed to run on all
> machines and the proper thing to do if a hardware feature is not
> available is to SKIP the test and return 0.

Ah, right. The test should be just *skipped* on non-AMX or even 
non-XSAVE systems.

Will follow up with a separate patch.

Thanks,
Chang

[tip: x86/urgent] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test

[tip-bot2 for Chang S. Bae]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     20df737561484cb2d42e537663c03a7311d2b3c1
Gitweb:        https://git.kernel.org/tip/20df737561484cb2d42e537663c03a7311d2b3c1
Author:        Chang S. Bae <chang.seok.bae@intel.com>
AuthorDate:    Sat, 29 Jan 2022 09:36:47 -08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 23 Mar 2022 21:28:34 +01:00

selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test

Update the arch_prctl test to check the permission bitmap whether the
requested feature is added as expected or not.

Every non-dynamic feature that is enabled is permitted already for use.
TILECFG is not dynamic feature. Ensure the bit is always on from
ARCH_GET_XCOMP_PERM.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20220129173647.27981-3-chang.seok.bae@intel.com

---
 tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c
index 3615ef4..2189f03 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -368,9 +368,16 @@ static void req_xtiledata_perm(void)
 
 static void validate_req_xcomp_perm(enum expected_result exp)
 {
-	unsigned long bitmask;
+	unsigned long bitmask, expected_bitmask;
 	long rc;
 
+	rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
+	if (rc) {
+		fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
+	} else if (!(bitmask & XFEATURE_MASK_XTILECFG)) {
+		fatal_error("ARCH_GET_XCOMP_PERM returns XFEATURE_XTILECFG off.");
+	}
+
 	rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_PERM, XFEATURE_XTILEDATA);
 	if (exp == FAIL_EXPECTED) {
 		if (rc) {
@@ -383,10 +390,15 @@ static void validate_req_xcomp_perm(enum expected_result exp)
 		fatal_error("ARCH_REQ_XCOMP_PERM saw unexpected failure.\n");
 	}
 
+	expected_bitmask = bitmask | XFEATURE_MASK_XTILEDATA;
+
 	rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
 	if (rc) {
 		fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
-	} else if (bitmask & XFEATURE_MASK_XTILE) {
+	} else if (bitmask != expected_bitmask) {
+		fatal_error("ARCH_REQ_XCOMP_PERM set a wrong bitmask: %lx, expected: %lx.\n",
+			    bitmask, expected_bitmask);
+	} else {
 		printf("\tARCH_REQ_XCOMP_PERM is successful.\n");
 	}
 }

ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[Paolo Bonzini]

Thomas, Dave,

can this series be included in 5.18 and CCed to stable?

The bug makes the __state_perm field completely wrong.  As a result, 
arch_prctl(ARCH_GET_XCOMP_PERM) only returns the features that were 
requested last, forgetting what was already in __state_perm (the 
"permitted" argument to __xstate_request_perm).

In KVM, it is a bit worse.  It affects 
arch_prctl(ARCH_GET_XCOMP_GUEST_PERM) in the same way and also 
ioctl(KVM_GET_SUPPORTED_CPUID), but the bug can also make KVM return the 
wrong xsave state size to userspace.  It's likely to go unnoticed by 
userspace until Intel adds non-dynamic states above a dynamic one, but 
potentially userspace could allocate a buffer that is too small.

Paolo

On 1/29/22 18:36, Chang S. Bae wrote:
> Changes from V3:
> * Rebased onto 5.17-rc1.
> 
> V3: https://lore.kernel.org/lkml/20211110003209.21666-1-chang.seok.bae@intel.com/
> 
> ---
> 
> The recent x86 dynamic state support incorporates the arch_prctl option to
> request permission before using a dynamic state.
> 
> It was designed to add the requested feature in the group leader's
> permission bitmask so that every thread can reference this master bitmask.
> The group leader is assumed to be unchanged here. The mainline is the case
> as a group leader is identified at fork() or exec() time only.
> 
> This master bitmask should include non-dynamic features always, as they
> are permitted by default. Users may check them via ARCH_GET_XCOMP_PERM.
> 
> But, in hindsight, the implementation does overwrite the bitmask with the
> requested bit only, instead of adding the bit to the existing one. This
> overwrite effectively revokes the permission that is granted already.
> 
> Fix the code and also update the selftest to disclose the issue if there
> is.
> 
> Chang S. Bae (1):
>    selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
> 
> Yang Zhong (1):
>    x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
> 
>   arch/x86/kernel/fpu/xstate.c      |  2 +-
>   tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
>   2 files changed, 15 insertions(+), 3 deletions(-)
> 
> 
> base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07

Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[Thomas Gleixner]

Paolo,

On Wed, Mar 23 2022 at 12:04, Paolo Bonzini wrote:
> can this series be included in 5.18 and CCed to stable?

working on it. There is another issue with that which I'm currently
looking into.

Thanks,

        tglx

Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[Thomas Gleixner]

On Wed, Mar 23 2022 at 13:27, Thomas Gleixner wrote:
> On Wed, Mar 23 2022 at 12:04, Paolo Bonzini wrote:
>> can this series be included in 5.18 and CCed to stable?
>
> working on it. There is another issue with that which I'm currently
> looking into.

The size calculation for the kernel state fails to take supervisor
states into account. Up to 5.18 that did not matter because ENQCMD/PASID
was disabled. But now it matters...

Thanks,

        tglx
---

--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
 
 	/* Calculate the resulting kernel state size */
 	mask = permitted | requested;
+	/* Take supervisor states into account */
+	mask |= xfeatures_mask_supervisor();
 	ksize = xstate_calculate_size(mask, compacted);
 
 	/* Calculate the resulting user state size */

Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[Paolo Bonzini]

On 3/23/22 13:55, Thomas Gleixner wrote:
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
>   
>   	/* Calculate the resulting kernel state size */
>   	mask = permitted | requested;
> +	/* Take supervisor states into account */
> +	mask |= xfeatures_mask_supervisor();
>   	ksize = xstate_calculate_size(mask, compacted);
>   

This should be only added in for the !guest case.

Paolo

Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[Thomas Gleixner]

On Wed, Mar 23 2022 at 15:24, Paolo Bonzini wrote:
> On 3/23/22 13:55, Thomas Gleixner wrote:
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
>>   
>>   	/* Calculate the resulting kernel state size */
>>   	mask = permitted | requested;
>> +	/* Take supervisor states into account */
>> +	mask |= xfeatures_mask_supervisor();
>>   	ksize = xstate_calculate_size(mask, compacted);
>>   
>
> This should be only added in for the !guest case.

Yes, I figured that out already :)

Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

[Thomas Gleixner]

On Wed, Mar 23 2022 at 18:07, Thomas Gleixner wrote:
> On Wed, Mar 23 2022 at 15:24, Paolo Bonzini wrote:
>> On 3/23/22 13:55, Thomas Gleixner wrote:
>>> --- a/arch/x86/kernel/fpu/xstate.c
>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>> @@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
>>>   
>>>   	/* Calculate the resulting kernel state size */
>>>   	mask = permitted | requested;
>>> +	/* Take supervisor states into account */
>>> +	mask |= xfeatures_mask_supervisor();
>>>   	ksize = xstate_calculate_size(mask, compacted);
>>>   
>>
>> This should be only added in for the !guest case.
>
> Yes, I figured that out already :)

Hrm, that has more consequences vs. the buffer conversion functions. Let
me stare some more.

Thanks,

        tglx