[v4] x86: Fix ARCH_REQ_XCOMP_PERM and update the test

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

Posted by Chang S. Bae 4 years, 5 months ago

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

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

Posted by Paolo Bonzini 4 years, 3 months ago

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