linux-kernel - RE: [patch 1/6] x86/fpu: Extend fpu_xstate

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: <BN9PR11MB5276B6158AC37CA38201D4308C759@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Tue, 14 Dec 2021 05:13:03 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>
CC:     Jing Liu <jing2.liu@...ux.intel.com>,
        "Zhong, Yang" <yang.zhong@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Nakajima, Jun" <jun.nakajima@...el.com>
Subject: RE: [patch 1/6] x86/fpu: Extend fpu_xstate_prctl() with guest
 permissions

> From: Thomas Gleixner <tglx@...utronix.de>
> Sent: Tuesday, December 14, 2021 10:50 AM
> 
> KVM requires a clear separation of host user space and guest permissions
> for dynamic XSTATE components.
> 
> Add a guest permissions member to struct fpu and a separate set of prctl()
> arguments: ARCH_GET_XCOMP_GUEST_PERM and
> ARCH_REQ_XCOMP_GUEST_PERM.
> 
> The semantics are equivalent to the host user space permission control
> except for the following constraints:
> 
>   1) Permissions have to be requested before the first vCPU is created
> 
>   2) Permissions are frozen when the first vCPU is created to ensure
>      consistency. Any attempt to expand permissions via the prctl() after
>      that point is rejected.

A curiosity question. Do we allow the user to reduce permissions?

> @@ -477,6 +479,13 @@ struct fpu {
>  	struct fpu_state_perm		perm;
> 
>  	/*
> +	 * @guest_perm:
> +	 *
> +	 * Permission related information for guest pseudo FPUs
> +	 */

why calling it 'pseudo'? It's real FPU state managed by this series...

> @@ -1742,6 +1751,7 @@ long fpu_xstate_prctl(struct task_struct
>  	u64 __user *uptr = (u64 __user *)arg2;
>  	u64 permitted, supported;
>  	unsigned long idx = arg2;
> +	bool guest = false;
> 
>  	if (tsk != current)
>  		return -EPERM;
> @@ -1760,11 +1770,20 @@ long fpu_xstate_prctl(struct task_struct
>  		permitted &= XFEATURE_MASK_USER_SUPPORTED;
>  		return put_user(permitted, uptr);
> 
> +	case ARCH_GET_XCOMP_GUEST_PERM:
> +		permitted = xstate_get_guest_group_perm();
> +		permitted &= XFEATURE_MASK_USER_SUPPORTED;
> +		return put_user(permitted, uptr);

Similarly as done for ARCH_REQ_XCOMP_GUEST_PERM:

+	case ARCH_GET_XCOMP_GUEST_PERM:
+		guest = true;
+		fallthrough;
+
	case ARCH_GET_XCOMP_PERM:
		/*
		 * Lockless snapshot as it can also change right after the
		 * dropping the lock.
		 */
-		permitted = xstate_get_host_group_perm();
+		permitted = xstate_get_group_perm(guest);
		permitted &= XFEATURE_MASK_USER_SUPPORTED;
		return put_user(permitted, uptr);

So the comment about 'lockless' is shared by both.

> +
> +	case ARCH_REQ_XCOMP_GUEST_PERM:
> +		guest = true;
> +		fallthrough;
> +
>  	case ARCH_REQ_XCOMP_PERM:
>  		if (!IS_ENABLED(CONFIG_X86_64))
>  			return -EOPNOTSUPP;
> 
> -		return xstate_request_perm(idx);
> +		return xstate_request_perm(idx, guest);
> 
>  	default:
>  		return -EINVAL;

Thanks
Kevin