linux-kernel - RE: [PATCH v6 04/21] kvm: x86: Exclude unpermitted xfeatures at KVM_GET_SUPPORTED

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] [day] [month] [year] [list]

Message-ID: <BN9PR11MB5276A817FEC26CAFCCE11CEA8C5E9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Mon, 24 Jan 2022 07:18:20 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Like Xu <like.xu.linux@...il.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "Liu, Jing2" <jing2.liu@...el.com>
CC:     "Zeng, Guang" <guang.zeng@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Wang, Wei W" <wei.w.wang@...el.com>,
        "Zhong, Yang" <yang.zhong@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        kvm list <kvm@...r.kernel.org>
Subject: RE: [PATCH v6 04/21] kvm: x86: Exclude unpermitted xfeatures at
 KVM_GET_SUPPORTED_CPUID

> From: Like Xu <like.xu.linux@...il.com>
> Sent: Sunday, January 23, 2022 2:22 PM
> 
> On 8/1/2022 2:54 am, Paolo Bonzini wrote:
> > From: Jing Liu <jing2.liu@...el.com>
> >
> > KVM_GET_SUPPORTED_CPUID should not include any dynamic xstates in
> > CPUID[0xD] if they have not been requested with prctl. Otherwise
> > a process which directly passes KVM_GET_SUPPORTED_CPUID to
> > KVM_SET_CPUID2 would now fail even if it doesn't intend to use a
> > dynamically enabled feature. Userspace must know that prctl is
> > required and allocate >4K xstate buffer before setting any dynamic
> > bit.
> >
> > Suggested-by: Paolo Bonzini <pbonzini@...hat.com>
> > Signed-off-by: Jing Liu <jing2.liu@...el.com>
> > Signed-off-by: Yang Zhong <yang.zhong@...el.com>
> > Message-Id: <20220105123532.12586-5-yang.zhong@...el.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> > ---
> >   Documentation/virt/kvm/api.rst | 4 ++++
> >   arch/x86/kvm/cpuid.c           | 9 ++++++---
> >   2 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/virt/kvm/api.rst
> b/Documentation/virt/kvm/api.rst
> > index 6b683dfea8f2..f4ea5e41a4d0 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -1687,6 +1687,10 @@ userspace capabilities, and with user
> requirements (for example, the
> >   user may wish to constrain cpuid to emulate older hardware, or for
> >   feature consistency across a cluster).
> >
> > +Dynamically-enabled feature bits need to be requested with
> > +``arch_prctl()`` before calling this ioctl. Feature bits that have not
> > +been requested are excluded from the result.
> > +
> >   Note that certain capabilities, such as KVM_CAP_X86_DISABLE_EXITS, may
> >   expose cpuid features (e.g. MONITOR) which are not supported by kvm in
> >   its default configuration. If userspace enables such capabilities, it
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index f3e6fda6b858..eb52dde5deec 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -815,11 +815,13 @@ static inline int __do_cpuid_func(struct
> kvm_cpuid_array *array, u32 function)
> >   				goto out;
> >   		}
> >   		break;
> > -	case 0xd:
> > -		entry->eax &= supported_xcr0;
> > +	case 0xd: {
> > +		u64 guest_perm = xstate_get_guest_group_perm();
> > +
> > +		entry->eax &= supported_xcr0 & guest_perm;
> >   		entry->ebx = xstate_required_size(supported_xcr0, false);
> 
> If we choose to exclude unpermitted xfeatures in the entry->eax, why do
> we choose to expose the size of unpermitted xfeatures in ebx and ecx?
> 
> This seems to be an inconsistency, how about:
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 1bd4d560cbdd..193cbf56a5fa 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -888,12 +888,12 @@ static inline int __do_cpuid_func(struct
> kvm_cpuid_array
> *array, u32 function)
>   		}
>   		break;
>   	case 0xd: {
> -		u64 guest_perm = xstate_get_guest_group_perm();
> +		u64 supported_xcr0 = supported_xcr0 &
> xstate_get_guest_group_perm();
> 
> -		entry->eax &= supported_xcr0 & guest_perm;
> +		entry->eax &= supported_xcr0;
>   		entry->ebx = xstate_required_size(supported_xcr0, false);
>   		entry->ecx = entry->ebx;
> -		entry->edx &= (supported_xcr0 & guest_perm) >> 32;
> +		entry->edx &= supported_xcr0 >> 32;
>   		if (!supported_xcr0)
>   			break;
> 
> It also helps to fix the CPUID_D_1_EBX and later for (i = 2; i < 64; ++i);
> 
> Is there anything I've missed ?

No, you are correct. Would you please send out a formal fix?

> 
> >   		entry->ecx = entry->ebx;
> > -		entry->edx &= supported_xcr0 >> 32;
> > +		entry->edx &= (supported_xcr0 & guest_perm) >> 32;
> >   		if (!supported_xcr0)
> >   			break;
> >
> > @@ -866,6 +868,7 @@ static inline int __do_cpuid_func(struct
> kvm_cpuid_array *array, u32 function)
> >   			entry->edx = 0;
> >   		}
> >   		break;
> > +	}
> >   	case 0x12:
> >   		/* Intel SGX */
> >   		if (!kvm_cpu_cap_has(X86_FEATURE_SGX)) {