Received-SPF: pass (google.com: domain of linux-kernel+bounces-63888-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 13 Feb 2024 16:29:42 +0000
From: Oliver Upton <oliver.upton@linux.dev>
To: Marc Zyngier <maz@kernel.org>
Cc: Will Deacon <will@kernel.org>, kvmarm@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	Ricardo Koller <ricarkol@google.com>
Subject: Re: [RFC PATCH] KVM: arm64: Fix double-free following
 kvm_pgtable_stage2_free_unlinked()
Message-ID: <ZcuY9rdvGaJ66edx@linux.dev>
References: <20240212193052.27765-1-will@kernel.org>
 <Zcp8LcvsZiZVkNKe@linux.dev>
 <86cyt062jh.wl-maz@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <86cyt062jh.wl-maz@kernel.org>

On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote:
> On Mon, 12 Feb 2024 20:14:37 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote:
> > > kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
> > > root page of the sub-tree before returning, so remove the additional
> > > put_page() invocations in the callers.
> > > 
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Oliver Upton <oliver.upton@linux.dev>
> > > Cc: Ricardo Koller <ricarkol@google.com>
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > > ---
> > > 
> > > Hi folks,
> > > 
> > > Sending this as an RFC as I only spotted it from code inspection and I'm
> > > surprised others aren't seeing fireworks if it's a genuine bug. I also
> > > couldn't come up with a sensible Fixes tag, as all of:
> > > 
> > >  e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
> > >  8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
> > >  f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")
> 
> I'd blame it on the last commit, as we really ought to have it if we
> have the others.
> 
> > >
> > > are actually ok in isolation. Hrm. Please tell me I'm wrong?
> > > 
> > >  arch/arm64/kvm/hyp/pgtable.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > > index c651df904fe3..ab9d05fcf98b 100644
> > > --- a/arch/arm64/kvm/hyp/pgtable.c
> > > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > > @@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt,
> > >  				 level + 1);
> > >  	if (ret) {
> > >  		kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level);
> > > -		mm_ops->put_page(pgtable);
> > >  		return ERR_PTR(ret);
> > >  	}
> > 
> > AFAICT, this entire branch is effectively dead code, unless there's a
> > KVM bug lurking behind the page table walk. The sub-tree isn't visible
> > to other software or hardware walkers yet, so none of the PTE races
> > could cause this to pop.
> > 
> > So while this is very obviously a bug, it might be pure luck that folks
> > haven't seen smoke here. Perhaps while fixing the bug we should take the
> > opportunity to promote the condition to WARN_ON_ONCE().
> 
> Can't you construct a case where an allocation fails during the walk
> (memcache empty), and we end up on this exact path?

Possibly, but AFAICT that can only happen if there was a bug in KVM. We
don't start the walk at all if userspace set the split chunk size to 0,
and otherwise we expect it to be at least PMD_SIZE, which will top up
the cache to 1 every pass. stage2_split_walker() will 'do the right
thing' if there aren't enough preallocated pages to get down to level 3.

It really doesn't matter either way, I'm just trying to convince myself
of the reasons why we haven't seen this explode yet :)

> > 
> > > @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> > >  
> > >  	if (!stage2_try_break_pte(ctx, mmu)) {
> > >  		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
> > > -		mm_ops->put_page(childp);
> > >  		return -EAGAIN;
> > >  	}
> > 
> > This, on the other hand, seems possible. There exists a race where an
> > old block PTE could have the AF set on it and the underlying cmpxchg()
> > could fail. There shouldn't be a race with any software walkers, as we
> > hold the MMU lock for write here.
> 
> AF update is indeed a likely candidate.
> 
> In any case, this patch looks good to me as it is, and we can always
> have a separate tweak to adjust the severity of the first case as
> required. Unless anyone objects, I'd like to queue it shortly.

Agreed, happy with the way this looks and should've added:

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

the first time around.

-- 
Thanks,
Oliver