Re: [PATCH v2 3/6] KVM: arm64: ptdump: Fix UAF when mmu->pgt is freed

[Leonardo Bras <leo.bras@arm.com>]

On Wed, Jul 01, 2026 at 06:27:51PM +0100, Wei-Lin Chang wrote:
> On Wed, Jul 01, 2026 at 04:00:41PM +0100, Leonardo Bras wrote:
> > Hi Wei Lin,
> > 
> > On Tue, Jun 30, 2026 at 01:10:02PM +0100, Wei-Lin Chang wrote:
> > > ptdump files can still be read after the pgt of the canonical mmu is
> > > freed, if they are opened before the VM debugfs directory is removed.
> > > This triggers UAF in places where we cache the pgt pointer or access it
> > > without checking its validity.
> > > 
> > > Check the pgt is still alive under the mmu_lock before accessing the
> > > pgt.
> > > 
> > > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > Closes: https://sashiko.dev/#/patchset/20260623142443.648972-1-weilin.chang@arm.com?part=1
> > > Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
> > > ---
> > >  arch/arm64/kvm/ptdump.c | 38 ++++++++++++++++++++++++--------------
> > >  1 file changed, 24 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> > > index d5aa9eff08d1..752d8e0cd25c 100644
> > > --- a/arch/arm64/kvm/ptdump.c
> > > +++ b/arch/arm64/kvm/ptdump.c
> > > @@ -115,13 +115,21 @@ static int kvm_ptdump_build_levels(struct ptdump_pg_level *level, u32 start_lvl)
> > >  static struct kvm_ptdump_guest_state *kvm_ptdump_parser_create(struct kvm *kvm)
> > >  {
> > >  	struct kvm_ptdump_guest_state *st;
> > > -	struct kvm_pgtable *pgtable = kvm->arch.mmu.pgt;
> > > +	struct kvm_pgtable *pgtable;
> > >  	int ret;
> > >  
> > >  	st = kzalloc_obj(struct kvm_ptdump_guest_state, GFP_KERNEL_ACCOUNT);
> > >  	if (!st)
> > >  		return ERR_PTR(-ENOMEM);
> > >  
> > > +	guard(write_lock)(&kvm->mmu_lock);
> > > +	if (!kvm->arch.mmu.pgt) {
> > > +		kfree(st);
> > > +		return ERR_PTR(-ENXIO);
> > > +	}
> > > +
> > > +	pgtable = kvm->arch.mmu.pgt;
> > > +
> > >  	ret = kvm_ptdump_build_levels(&st->level[0], pgtable->start_level);
> > >  	if (ret) {
> > >  		kfree(st);
> > > @@ -137,7 +145,6 @@ static struct kvm_ptdump_guest_state *kvm_ptdump_parser_create(struct kvm *kvm)
> > >  
> > >  static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
> > >  {
> > > -	int ret;
> > >  	struct kvm_ptdump_guest_state *st = m->private;
> > >  	struct kvm *kvm = st->kvm;
> > >  	struct kvm_s2_mmu *mmu = &kvm->arch.mmu;
> > > @@ -154,11 +161,11 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
> > >  		.seq		= m,
> > >  	};
> > >  
> > > -	write_lock(&kvm->mmu_lock);
> > > -	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> > > -	write_unlock(&kvm->mmu_lock);
> > > +	guard(write_lock)(&kvm->mmu_lock);
> > > +	if (mmu->pgt)
> > > +		return kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> > 
> > IIUC, that's the same behavior, right?
> > Just changed to look about the same with the rest of this file?
> 
> I'm not too sure what you are referring to, if you mean the
> write_lock/unlock -> guard(write_lock) change, then yes, mostly. Just
> also checking mmu->pgt is still not freed.

I was referring just to lock+unlock -> guard() strategy.
IIUC this has the same effect, so I was wondering on why you suggested the 
change.

I then supposed it was to look the same of the rest of the file.

> 
> > 
> > >  
> > > -	return ret;
> > > +	return 0;
> > >  }
> > 
> > So if the pgt does not exist anymore, it returns zero. Is that the desired 
> > behavior?
> 
> Good question, so the question is what contract between the ptdump and
> user do we want to make for this case. I guess returning some error like -EIO
> could make a little more sense than just printing nothing?
>

It depends on what is the behaviour you want to see... see below..
 
> > 
> > I guess it's aligned with the idea of single file mentioned in the cover, 
> > right?
> 
> Sorry, I don't get what you are asking here?

IIUC you mentioned in the cover letter that you planned to have a file 
which would, on read, output ptdump for every nested pgtable. Did I get 
that right? 

If so, I imagine that the user has no idea how many nested pgtables are 
there, and if the ioctl initially finds N, but ends up printing N-1 
(because a pgtable was not there anymore), that would not be an error that 
the user should be worried about.

It could be an issue if you were printing in multiple steps (multiple 
checks), as a part of that pgtable could be printed while another part 
could not. Which does not seem to be the case.

> 
> > 
> > >  
> > >  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
> > > @@ -206,17 +213,23 @@ static const struct file_operations kvm_ptdump_guest_fops = {
> > >  
> > >  static int kvm_pgtable_range_show(struct seq_file *m, void *unused)
> > >  {
> > > -	struct kvm_pgtable *pgtable = m->private;
> > > +	struct kvm *kvm = m->private;
> > > +
> > > +	guard(write_lock)(&kvm->mmu_lock);
> > > +	if (kvm->arch.mmu.pgt)
> > > +		seq_printf(m, "%2u\n", kvm->arch.mmu.pgt->ia_bits);
> > >  
> > > -	seq_printf(m, "%2u\n", pgtable->ia_bits);
> > >  	return 0;
> > >  }
> > >  
> > >  static int kvm_pgtable_levels_show(struct seq_file *m, void *unused)
> > >  {
> > > -	struct kvm_pgtable *pgtable = m->private;
> > > +	struct kvm *kvm = m->private;
> > > +
> > > +	guard(write_lock)(&kvm->mmu_lock);
> > > +	if (kvm->arch.mmu.pgt)
> > > +		seq_printf(m, "%1d\n", KVM_PGTABLE_MAX_LEVELS - kvm->arch.mmu.pgt->start_level);
> > >  
> > > -	seq_printf(m, "%1d\n", KVM_PGTABLE_MAX_LEVELS - pgtable->start_level);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -224,15 +237,12 @@ static int kvm_pgtable_debugfs_open(struct inode *m, struct file *file,
> > >  				    int (*show)(struct seq_file *, void *))
> > >  {
> > >  	struct kvm *kvm = m->i_private;
> > > -	struct kvm_pgtable *pgtable;
> > >  	int ret;
> > >  
> > >  	if (!kvm_get_kvm_safe(kvm))
> > >  		return -ENOENT;
> > >  
> > > -	pgtable = kvm->arch.mmu.pgt;
> > > -
> > > -	ret = single_open(file, show, pgtable);
> > > +	ret = single_open(file, show, kvm);
> > 
> > Maybe this change is more related with the previous patch?
> 
> I see your point, but I divided it into first fixing mmu UAF, then the
> pgt UAF, which I also think makes sense.
> 

Okay then

Thanks!
Leo