linux-kernel - Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2a2e7a8a-6cd3-1392-b080-54161b990ff0@intel.com>
Date:   Mon, 7 Mar 2022 10:53:59 -0800
From:   "Chang S. Bae" <chang.seok.bae@...el.com>
To:     Hao Xiang <hao.xiang@...ux.alibaba.com>, <yang.zhong@...el.com>
CC:     <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
        <linux-kernel@...r.kernel.org>, <mingo@...hat.com>,
        <ravi.v.shankar@...el.com>, <tglx@...utronix.de>, <x86@...nel.org>
Subject: Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM
 implementation

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