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

[Wei-Lin Chang]

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 at kernel.org>
> > Closes: https://sashiko.dev/#/patchset/20260623142443.648972-1-weilin.chang@arm.com?part=1
> > Signed-off-by: Wei-Lin Chang <weilin.chang at 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.

> 
> >  
> > -	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?

> 
> 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?

> 
> >  
> >  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.

Thanks,
Wei-Lin Chang

> 
> >  	if (ret < 0)
> >  		kvm_put_kvm(kvm);
> >  	return ret;
> > -- 
> > 2.43.0
> > 
> 
> Thanks!
> Leo