From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6BB5D3A7591; Mon, 8 Jun 2026 09:41:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780911687; cv=none; b=tb16uMP2fiVIf0jddmkEJczws2eAZGd6kMkaNiJIp7v2oXbOkwmD8dYZYwwj6KO7CvosSVOZuzz6Ezy+d/jFNH2YRvJN31va84tQbbebcAd42CpPFafEjia2Fg1vFvv9UVNJownt4/gtqDnaj/Cl0ZRBG2kBm6ruBe7rO6iofbA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780911687; c=relaxed/simple; bh=e9VGB3RzJNeMtY5Q7FpVM9If4E09QwmbsI6rjIqlMbA=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=BqUKyWYyluiV+Yq1U5bH5N5HXH/RZt8RYIH1iyKhUw1aom6KqQxQCmhG9D9kYNYqjB3GxreDMZNcd9wDf58pRccrVmuX5Kx4howxcm3isaj4OSPwTQ9MB6wVSzCG+MaDUT01zQLUaAdcx8CJnpU6haIf+YpqyIGW2nYwn5EI+I8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=gCl2iP1r; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="gCl2iP1r" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C07611F00898; Mon, 8 Jun 2026 09:41:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780911684; bh=+sKBIXnDcUDiKipp4dXaJfavVK02CQvHaodclEeqrTM=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=gCl2iP1rK5kBMVwYVpj1i15Uep2qUGOmKN4PdWLCQ2tb0MpsP0cMYUU4BAl8rm6SG zn1anTo8dZqC6to6HLnwfsZslT76uvFxD2pVow6hOHAagMEO8GlgdULXwWfc3rEIyX X73VKVr5BauzY3y6pNldxJBHyFAtT13BsBN0g8VtppZ3k9W5Rv6kEIyGxzhj2fi8sp hiY+2+yaxAR7pKyhgp6y+ha6bGrkg8x36YGcT6+2XyDSZmG4hFlAb7k0yFeSRmjGXz ar5hvF4ajT6m7YfMO4AZ8n82lz/5YuLDaM0K9vERCMLGq/UEWkR/ifOhoygYBlzXsD VeGSFGsvAJPZg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1wWWTz-0000000APSs-02nj; Mon, 08 Jun 2026 09:41:23 +0000 Date: Mon, 08 Jun 2026 10:41:22 +0100 Message-ID: <86tsrdtlml.wl-maz@kernel.org> From: Marc Zyngier To: sashiko-reviews@lists.linux.dev Cc: kvm@vger.kernel.org Subject: Re: [PATCH] KVM: arm64: Hold kvm->mmu_lock while initialising vcpu->arch.vncr_tlb In-Reply-To: <20260608082603.16AEC1F00893@smtp.kernel.org> References: <20260608081108.2244133-1-maz@kernel.org> <20260608082603.16AEC1F00893@smtp.kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: sashiko-reviews@lists.linux.dev, kvm@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Mon, 08 Jun 2026 09:26:02 +0100, sashiko-bot@kernel.org wrote: > > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: > > Pre-existing issues: > - [Critical] Missing NULL check for `vcpu->arch.vncr_tlb` in `invalidate_vncr_va()` causes a host kernel panic when iterating over uninitialized vCPUs. > - [High] The lazily allocated `vncr_tlb` structure leaves the `cpu` field zero-initialized, causing it to inadvertently unmap CPU 0's VNCR fixmap on the first fault. > - [High] Resource leak of `struct page` references on error paths in `kvm_translate_vncr()`. > -- > > commit cf6d236759b9eb5ba69620ec32a536af7c15a9a2 > Author: Marc Zyngier > > KVM: arm64: Hold kvm->mmu_lock while initialising vcpu->arch.vncr_tlb > > This commit fixes a race condition and memory leak in the initialization > of vncr_tlb by taking the mmu_lock during assignment and verifying it is > NULL beforehand. > > Link: https://lore.kernel.org/r/20260607180815.85FBC1F00893@smtp.kernel.org > > > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c > > index 883b6c1008fbb..21c9d1f07d7a8 100644 > > --- a/arch/arm64/kvm/nested.c > > +++ b/arch/arm64/kvm/nested.c > > @@ -1248,8 +1248,14 @@ int kvm_vcpu_allocate_vncr_tlb(struct kvm_vcpu *vcpu) > > if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY)) > > return 0; > > > > - vcpu->arch.vncr_tlb = kzalloc_obj(*vcpu->arch.vncr_tlb, > > - GFP_KERNEL_ACCOUNT); > > + if (!vcpu->arch.vncr_tlb) { > > + struct vncr_tlb *vt = kzalloc_obj(*vcpu->arch.vncr_tlb, > > + GFP_KERNEL_ACCOUNT); > > [Severity: High] > This is a pre-existing issue, but since we are allocating the vncr_tlb here > using kzalloc_obj(), vt->cpu will be zero-initialized. > > Looking at invalidate_vncr(), if a vCPU encounters its first VNCR fault, > kvm_translate_vncr() calls invalidate_vncr(vt). Because vt->cpu is 0 > instead of -1, the following check passes: > > arch/arm64/kvm/nested.c:invalidate_vncr() { > ... > if (vt->cpu != -1) > clear_fixmap(vncr_fixmap(vt->cpu)); > ... > } > > Does this inadvertently unmap CPU 0's fixmap, potentially corrupting the > translation state if another vCPU is actively mapped on CPU 0? Could we > initialize vt->cpu to -1 right after this allocation? This is only evaluated when valid is true, and this is checked in the calling context. Since valid is initialised to false by virtue of the structure being 0-initialised, this path doesn't look possible. > > > + > > + scoped_guard(write_lock, &vcpu->kvm->mmu_lock) > > + vcpu->arch.vncr_tlb = vt; > > + } > > + > > if (!vcpu->arch.vncr_tlb) > > return -ENOMEM; > > [Severity: Critical] > This isn't a bug introduced by this patch, but while reviewing the vncr_tlb > lifecycle, I noticed a potential issue in invalidate_vncr_va() when > iterating over uninitialized vCPUs. > > If an online vCPU triggers an EL2 TLBI instruction before another vCPU in > the same VM has run, could this lead to a NULL pointer dereference? > > arch/arm64/kvm/nested.c:invalidate_vncr_va() { > ... > kvm_for_each_vcpu(i, vcpu, kvm) { > struct vncr_tlb *vt = vcpu->arch.vncr_tlb; > > if (!vt->valid) > continue; > ... > } > > Since vncr_tlb is allocated lazily on the first run, vt could be NULL here. > Does this path need a NULL check before accessing vt->valid? > This is already fixed by a separate patch (https://lore.kernel.org/all/20260607175745.297793-1-maz@kernel.org/) > [Severity: High] > This is also a pre-existing issue, but I noticed a potential page reference > leak in kvm_translate_vncr() on the error paths. > > When __kvm_faultin_pfn() successfully pins a page, the early return for a > write fault without a writable page bypasses kvm_release_faultin_page(): > > arch/arm64/kvm/nested.c:kvm_translate_vncr() { > ... > if (is_error_noslot_pfn(pfn) || (write_fault && !writable)) > return -EFAULT; > ... > } This looks like a real issue indeed. > Similarly, the MMU retry check also returns directly: > > arch/arm64/kvm/nested.c:kvm_translate_vncr() { > ... > if (mmu_invalidate_retry(vcpu->kvm, mmu_seq)) > return -EAGAIN; > ... > } This one has been fixed already: https://patch.msgid.link/20260602235450.103057-2-oupton@kernel.org M. -- Without deviation from the norm, progress is not possible.