Received-SPF: pass (google.com: domain of linux-kernel+bounces-94789-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <baea57a0-a50e-41ce-a6a6-4e52c5c3d486@intel.com>
Date: Thu, 7 Mar 2024 13:46:04 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 16/16] KVM: x86/mmu: Sanity check that __kvm_faultin_pfn()
 doesn't create noslot pfns
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>, Paolo Bonzini
	<pbonzini@redhat.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Yan Zhao
	<yan.y.zhao@intel.com>, Isaku Yamahata <isaku.yamahata@intel.com>, "Michael
 Roth" <michael.roth@amd.com>, Yu Zhang <yu.c.zhang@linux.intel.com>, Chao
 Peng <chao.p.peng@linux.intel.com>, Fuad Tabba <tabba@google.com>, David
 Matlack <dmatlack@google.com>
References: <20240228024147.41573-1-seanjc@google.com>
 <20240228024147.41573-17-seanjc@google.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240228024147.41573-17-seanjc@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?b3lDTS8wWWppOVZyVkJKcVpIU3RES0Z4QkxDRWFmUy8rYzc5WUoweGNKekgw?=
 =?utf-8?B?QkpDak1XS0sxMDBYZmtPS2NaQllHZnVRbjJtUmxNeGRZZHh3VnFjTUF1UzJO?=
 =?utf-8?B?RjZvbU4waC84VG5mVTlMUnA4U3g1OGNOQldhNlB2TG9YL0Rpb0NNSmZCa1Jl?=
 =?utf-8?B?VmlWS1dlRGN4SGdGa2FIeGVSTmFWaTFtdGFSSTBwZlUycHZGLytweU9jdnZ3?=
 =?utf-8?B?U3FNMnZuN2ZpVGpoV0d4ek1sZ28xNlFjajQzU2piVkNEd0NWai9Od0ljcDYw?=
 =?utf-8?B?NHpnWkJWRWFjY0Z4YVpPYnkyYk9SMnhjekhvd3RrMG54SkUzaFU4Zms1dERM?=
 =?utf-8?B?SmVKTDVLM3F3MnlJSUFrd3lxNnVWVUhrTVFTQloxSHg4Ym96TUExeEFUbzRw?=
 =?utf-8?B?YUhMdWJCdCt6RHYveHh5Q3dIbkhjUlFvd0ZMZ2JzcHk2QWhBMmFrSDJEemxz?=
 =?utf-8?B?di8wT0ZnSXIwdGNzcnNncGJzLzlmdzZKNVFYQkFKckxqYW13Q0Jpc3ltWi8x?=
 =?utf-8?B?QmtNNXJoNjJPbHZhbFpka3JrVWN5T3NkSDdFZ2U2Z1dUWm0vS0htd3VBb2lO?=
 =?utf-8?B?L3BTYlh1bXRpcHZtY2RtVkRreGtrQmcvWk40WXRhb2hnZEpYRFlPK3h0SHlS?=
 =?utf-8?B?MUVHUlNDRWV2VDNDRUZrbmZOTWt1S2MyYU93QzZZcnVVN2FUZ2hEWjE1NjdW?=
 =?utf-8?B?ZmYzcUUzQmRRRmRNU29vbisyV0dka2xoRGtlMHg4Z01rQjVqSkREWFJSQTRy?=
 =?utf-8?B?VnFXVFR1ZmtwWmVZeXpXd0xVSGFteWpSZVh6VmVVMVlrcnVja3VKcGhNTVQr?=
 =?utf-8?B?cVNCNkg4dmdMVVNDRUcxa2tRWDhpQ2xZVkxpV3BGVmZhVkY0L1ROZCtxenhF?=
 =?utf-8?B?ZU10RENqRG53Wjd4dnNRVVhGNDNJcU5saVd4b28xUFE0WG5jUENkOGozcFQ1?=
 =?utf-8?B?OHZRNWpVbHVpSUY4WXloU01GMGQ1MlZ3M0I3Y0tud1pOUzhRcUlXRDRHVkxS?=
 =?utf-8?B?T0JobE82MklTSkpUOGlvcXZUejc3cVhYSlA2TGJhZVBkOEZGazBhUlRSMlpj?=
 =?utf-8?B?eXd3MHUxeFYzWjBqbndpUzJvcFhPTUN1Q0pCZkpEbkxaZ09HemlEWitTQ2ZT?=
 =?utf-8?B?WkoyV2QzeUVUVnkyZVo0QTI5SUZFS2pGSDF2UEVTNzE4RDlMMXVkdTlGUm5Q?=
 =?utf-8?B?c2tNdWwwL2p3NkUvUW5NbGNkYW1hSFNuazVPaGtXL0VTRm1jTFZtcDZ6ZHZn?=
 =?utf-8?B?U3hnZmcrdnhXWllvOXgrQjdKNmgzUzd0WDFKOU4yMzRmMlZodDg1UFBvdzdS?=
 =?utf-8?B?emRhb0phSXdOYzM0aVNYdG9JRG1xV2VYaklXZXhCK0VhNzcyTFRzdmdBQkNn?=
 =?utf-8?B?YWdtTE5GdEd2d0tHZkY1RWpMTkVPWW93NDZ1RVp1UGlWME9TclJnNm1QOWRr?=
 =?utf-8?B?NmlVOHUwdzZpemVMZXd2ckRVRXVFSC9vYmc5N2lxdUlEdFFJaVg5VFV1MFlz?=
 =?utf-8?B?dzY3eDlSdUJsSGs5Yjlobmh4eXhRTlFzdDRCUjd5MGp5Kzk0QjlrUEwwRGp3?=
 =?utf-8?B?SG1WT29TMjFQS2UwMVBZZnIzSlpDWHJvR3puckJBWGlNMm1GbTk2N0Zsckd1?=
 =?utf-8?B?cTBOMDJJQWtrTm0wVjRRTVIwSDR5Y1JxV01pQng5RmlXWkJwaEc0MXVJaWJo?=
 =?utf-8?B?TWRZNFFRNXZydnAzNllzMW5QM2hmZmdTai9jeWN4elcxTVpTcnBqZENTY3ND?=
 =?utf-8?B?NGhxa1ZjOUpwTmIvMVdJSzdqaXUvaW5XZU9yRGRJejBqVy9XZmNXUEVMNGlx?=
 =?utf-8?B?RVlDK0MvMDRJZm1XRDdEdkd3akZ5WmVMaG9wYUU1elpSUjMyL0JsUnR3Z1dO?=
 =?utf-8?B?aThubFAyUFdRNlFMWm8vZmtQb1FuQzE3eGpVUEtjQUU0SmorNDl2K29OWWdP?=
 =?utf-8?B?Q1l0T0RJd3h6bjNQdXpjM1l5MFNyRnFmL3BiQ3hxR00xSSt4WVlkQ1ZYM3FY?=
 =?utf-8?B?TVNmTzhiY3k4bFY1RmVFM2lzVklVZVd5V3hkeTNyYnBNZFNMUzNVSXN5Tjd2?=
 =?utf-8?B?K2MvREpTYm1MQmVGZGVQRDJ0bjM4WERDSHkzNlNRYnRQeng3cjJCM0g2dzJB?=
 =?utf-8?B?OFExV29MQVNDNVhPa1FsMHlVUnRhNTlNdHVkbzE4aytUdnhrWVZmNGNTZ2Nj?=
 =?utf-8?B?M3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b15a9899-a2fe-4b10-23a3-08dc3e3ffd4f
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2024 00:46:13.4957
 (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: jMX65C4EYYCoXZ9RzOid52/gVgISbfNVN98t0l315suVFfgAR+dxPTeOXUHkOO95VAhiSUx32zs6y6Awpyzf3A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6728
X-OriginatorOrg: intel.com


On 28/02/2024 3:41 pm, Sean Christopherson wrote:
> WARN if __kvm_faultin_pfn() generates a "no slot" pfn, and gracefully
> handle the unexpected behavior instead of continuing on with dangerous
> state, e.g. tdp_mmu_map_handle_target_level() _only_ checks fault->slot,
> and so could install a bogus PFN into the guest.
> 
> The existing code is functionally ok, because kvm_faultin_pfn() pre-checks
> all of the cases that result in KVM_PFN_NOSLOT, but it is unnecessarily
> unsafe as it relies on __gfn_to_pfn_memslot() getting the _exact_ same
> memslot, i.e. not a re-retrieved pointer with KVM_MEMSLOT_INVALID set.
> And checking only fault->slot would fall apart if KVM ever added a flag or
> condition that forced emulation, similar to how KVM handles writes to
> read-only memslots.
> 
> Cc: David Matlack <dmatlack@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/mmu/mmu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 43f24a74571a..cedacb1b89c5 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4468,7 +4468,7 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
>   	if (unlikely(is_error_pfn(fault->pfn)))
>   		return kvm_handle_error_pfn(vcpu, fault);
>   
> -	if (WARN_ON_ONCE(!fault->slot))
> +	if (WARN_ON_ONCE(!fault->slot || is_noslot_pfn(fault->pfn)))
>   		return kvm_handle_noslot_fault(vcpu, fault, access);
>   
>   	/*

Reviewed-by: Kai Huang <kai.huang@intel.com>