Received-SPF: pass (google.com: domain of linux-kernel+bounces-98401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 11 Mar 2024 09:16:41 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, Lai Jiangshan
	<jiangshanlai@gmail.com>, "Paul E. McKenney" <paulmck@kernel.org>, "Josh
 Triplett" <josh@joshtriplett.org>, <kvm@vger.kernel.org>,
	<rcu@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Kevin Tian
	<kevin.tian@intel.com>, Yiwei Zhang <zzyiwei@google.com>
Subject: Re: [PATCH 5/5] KVM: VMX: Always honor guest PAT on CPUs that
 support self-snoop
Message-ID: <Ze5bee/qJ41IESdk@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240309010929.1403984-1-seanjc@google.com>
 <20240309010929.1403984-6-seanjc@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240309010929.1403984-6-seanjc@google.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?R41UPWtVAWsgSDTlhVmjTloATtpNeofiAzA75I6wO1VALrwFd4K+qKJ2kxp+?=
 =?us-ascii?Q?iV+48I/yEADH1DgYumEQPVd+qBxlNSz9yYH94dZQvDMfWvLvQmoFYoSolrnk?=
 =?us-ascii?Q?VPaU0/Q8QvP9HPOIg4BIBUcFgenHi+jiqmbQOBnMWVUxirdQoxUdlzzzlcCB?=
 =?us-ascii?Q?1csurFKIyQslzwru3YhsYdDM1QvCXLXGZ/7du3SRdz3PDNy69+APicbT3iVn?=
 =?us-ascii?Q?lLF0NUZopZGf+H/Cp4irhC0sisf4XO/IYPQpM0TmbiKZXiPpq1PUMWv9WTV1?=
 =?us-ascii?Q?+SauE5eG4QHR9z7GC853eYbra/HZhtKj29UzGssfKUugmlESraQfrWKaB4ep?=
 =?us-ascii?Q?tC88NRtMw3GD2wtmUhNuKxbVF/3YDwtbuelLTwCDyWEr76vU5/lk9dJi5gOp?=
 =?us-ascii?Q?cLjYdm1OTeTItDLTjIa0o+nJqErg5UnsugmMiubSY9jVmjTORGavsOj0S68S?=
 =?us-ascii?Q?fTdtw/xALtq/tjZYswQsWFGAKe3ZT5aA3F/YXE80HNroeXJLrg8P6Z6DWZ5D?=
 =?us-ascii?Q?X/6pWnQxpqPw7iK3Cp0Dvvt+RZLYpnTA4dxlxX0T3jrA0kosSyPxJfomsWci?=
 =?us-ascii?Q?SyzElIYHeVPTEqa4CZzTYtKifYLH81r/lwXgUCeZc5Uuo4R+1S/4y2psLKQr?=
 =?us-ascii?Q?CI5scXqKlCnMo83PTUlQ2xzvc4D0neDhg5PeDaQntpGOK1BUwQzctTvQitD0?=
 =?us-ascii?Q?jWF0JB+hjEkihx10YAiJwXhnzoBSIUmjBFp0OazqKCD/wYqyUa8fhwxrEUgV?=
 =?us-ascii?Q?V3HFUdHHQEULXgSgfoN4renn1kNVHeiDotHf78beh72N6nBFdJJal7nZpQcb?=
 =?us-ascii?Q?+pU1M9+gqIfJao8ua7D1YoqXy1OGbrrzmmI3UvdBQfyHGLTUX5tOMFzsfCKl?=
 =?us-ascii?Q?ZjU4AOavVNFCUz5YKkoYiVDZwXsAstNIFiZ7QdttXiSIsSD1IjmkMLCkSxRL?=
 =?us-ascii?Q?7Yr54ZPp4zeek3Oj12H6TfraFPDSZ7EutFtSsYPoxLLXUZ9LM2KsijrUoEEs?=
 =?us-ascii?Q?EHXaw5gZX8dqmlQD+el8rrlPwItNL0PluLUl4uQi84JXvhFj8HpfN7UFVKj7?=
 =?us-ascii?Q?QOUXKBFjsXxsZwCRXtwvD5GStwLOdert38M5HFAzTn7OgDFecjLYSyimzl6i?=
 =?us-ascii?Q?4ZuA1N782f94xFbl9TXCuVItVw7M9jEfTrbuagmWV7epIU2fbnskA8aB3QYx?=
 =?us-ascii?Q?Xp0zzWrqPSBr/amGiq4OrEO1iEwsr4ACzllIb3xKpi1ivG89hSXEtDKFvuUl?=
 =?us-ascii?Q?0J/kautrsuv7GT4ZQQsUAgutEAu1bohDEcTypkbeShu/NSvEpGuM68b+kN6x?=
 =?us-ascii?Q?LX6Tq9TnfcSb5SBUozX+PUp7fiqqqp27wkx/VryXReipWgvz/NNI8VsuShFK?=
 =?us-ascii?Q?1xRg3Gn7gELleDpM6yLjr53V15/MAvVwK6SHXIEEE/skPe8QVat6u3Cv7iAC?=
 =?us-ascii?Q?STW8Ur0xlskUQApr8u9wcnIULvJSD5alKN5oRldVjEhd6Lxl+De7sjFz818t?=
 =?us-ascii?Q?cW+p2pKUZqxtSclK9fIqJKAwZWpdNFgXHkfuOmzqRsP7S6p3WdECOvSZIvl6?=
 =?us-ascii?Q?nbZ+BF9NeHQCpB2phH/FZdwSNL3oKFAfCjDuS2me?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ca375ec-1444-4f44-11dd-08dc416d1449
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2024 01:46:32.8285
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LYh00t6mp1M4TJ6fIjKpB1+eMOI9eUzJ1Q0MrHQf6VwwOogvnmdG6IZzEs2+W49H/PtB4DSu20gwpNSSkOcOqg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8404
X-OriginatorOrg: intel.com

On Fri, Mar 08, 2024 at 05:09:29PM -0800, Sean Christopherson wrote:
> Unconditionally honor guest PAT on CPUs that support self-snoop, as
> Intel has confirmed that CPUs that support self-snoop always snoop caches
> and store buffers.  I.e. CPUs with self-snoop maintain cache coherency
> even in the presence of aliased memtypes, thus there is no need to trust
> the guest behaves and only honor PAT as a last resort, as KVM does today.
> 
> Honoring guest PAT is desirable for use cases where the guest has access
> to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual
> (mediated, for all intents and purposes) GPU is exposed to the guest, along
> with buffers that are consumed directly by the physical GPU, i.e. which
> can't be proxied by the host to ensure writes from the guest are performed
> with the correct memory type for the GPU.
> 
> Cc: Yiwei Zhang <zzyiwei@google.com>
> Suggested-by: Yan Zhao <yan.y.zhao@intel.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mmu/mmu.c |  8 +++++---
>  arch/x86/kvm/vmx/vmx.c | 10 ++++++----
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 403cd8f914cd..7fa514830628 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4622,14 +4622,16 @@ static int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu,
>  bool kvm_mmu_may_ignore_guest_pat(void)
>  {
>  	/*
> -	 * When EPT is enabled (shadow_memtype_mask is non-zero), and the VM
> +	 * When EPT is enabled (shadow_memtype_mask is non-zero), the CPU does
> +	 * not support self-snoop (or is affected by an erratum), and the VM
>  	 * has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is to
>  	 * honor the memtype from the guest's PAT so that guest accesses to
>  	 * memory that is DMA'd aren't cached against the guest's wishes.  As a
>  	 * result, KVM _may_ ignore guest PAT, whereas without non-coherent DMA,
> -	 * KVM _always_ ignores guest PAT (when EPT is enabled).
> +	 * KVM _always_ ignores or honors guest PAT, i.e. doesn't toggle SPTE
> +	 * bits in response to non-coherent device (un)registration.
>  	 */
> -	return shadow_memtype_mask;
> +	return !static_cpu_has(X86_FEATURE_SELFSNOOP) && shadow_memtype_mask;
>  }
>  
>  int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 17a8e4fdf9c4..5dc4c24ae203 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7605,11 +7605,13 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
>  
>  	/*
>  	 * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
> -	 * device attached.  Letting the guest control memory types on Intel
> -	 * CPUs may result in unexpected behavior, and so KVM's ABI is to trust
> -	 * the guest to behave only as a last resort.
> +	 * device attached and the CPU doesn't support self-snoop.  Letting the
> +	 * guest control memory types on Intel CPUs without self-snoop may
> +	 * result in unexpected behavior, and so KVM's (historical) ABI is to
> +	 * trust the guest to behave only as a last resort.
>  	 */
> -	if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
> +	if (!static_cpu_has(X86_FEATURE_SELFSNOOP) &&
> +	    !kvm_arch_has_noncoherent_dma(vcpu->kvm))
>  		return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;

For the case of !static_cpu_has(X86_FEATURE_SELFSNOOP) &&
kvm_arch_has_noncoherent_dma(vcpu->kvm), I think we at least should warn
about unsafe before honoring guest memory type.
Though it's a KVM's historical ABI, it's not safe in the security perspective
because page aliasing without proper cache flush handling on CPUs without
self-snoop may open a door for guest to read uninitialized host data.
e.g. when there's a noncoherent DMA device attached, and if there's a memory
region that is not pinned in vfio/iommufd side, (e.g. memory region in vfio's
skipped section), then though the guest memory from this memory region is not
accessible to noncoherent DMAs, vCPUs can still access this part of guest memory.
Then if vCPUs use WC as guest type, it may bypass host's initialization data in
cache and read stale data in host, causing information leak.

My preference is still to force WB
(i.e. (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT) in case of
!static_cpu_has(X86_FEATURE_SELFSNOOP) && kvm_arch_has_noncoherent_dma(vcpu->kvm).
Firstly, it's because there're few CPUs with features VMX without self-snoop;
Secondly, security takes priority over functionality :)

>  
>  	return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT);
> -- 
> 2.44.0.278.ge034bb2e1d-goog
>