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

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

[Chang S. Bae]

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:
  (1) update each task's permission bitmask, instead of the group leader's. 
  (2) 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.

Reported-by: Yang Zhong <yang.zhong@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>

Chang S. Bae (2):
  x86/arch_prctl: Fix ARCH_REQ_XCOMP_PERM
  selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test

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


base-commit: 9a6cf455a952725422f4fb10848839989f833579
-- 
2.17.1

[PATCH 1/2] x86/arch_prctl: Fix ARCH_REQ_XCOMP_PERM

[Chang S. Bae]

ARCH_REQ_XCOMP_PERM is supposed to add the requested feature to the
permission bitmap of thread_group_leader()->fpu. The master permission
field is assumed to be used only instead of each task's fpu->perm field.

But the code updates the current task's bitmap instead of the group
leader's. It also does overwrite the bitmap with the requested feature bit
only rather than adding it.

Fix the code to add the request feature bit to the correct bitmap.

Reported-by: Yang Zhong <yang.zhong@intel.com>
Fixes: db8268df0983 ("x86/arch_prctl: Add controls for dynamic XSTATE components")
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
---
 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 d28829403ed0..f02c999049c8 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1626,7 +1626,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested)
 		return ret;
 
 	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
-	WRITE_ONCE(fpu->perm.__state_perm, requested);
+	WRITE_ONCE(current->group_leader->thread.fpu.perm.__state_perm, mask);
 	/* Protected by sighand lock */
 	fpu->perm.__state_size = ksize;
 	fpu->perm.__user_state_size = usize;
-- 
2.17.1

[PATCH 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 1/2] x86/arch_prctl: Fix ARCH_REQ_XCOMP_PERM

[Bae, Chang Seok]

On Nov 8, 2021, at 15:35, Bae, Chang Seok <chang.seok.bae@intel.com> wrote:
> But the code updates the current task's bitmap instead of the group
> leader's. 

This statement is wrong, sorry. The code already writes to the group leader’s.

V2 is here: 
    https://lore.kernel.org/lkml/20211109012122.1142-1-chang.seok.bae@intel.com/

Thanks,
Chang