[PATCH v2 3/6] KVM: arm64: ptdump: Fix UAF when mmu->pgt is freed
Wei-Lin Chang
weilin.chang at arm.com
Fri Jul 3 02:26:03 PDT 2026
On Thu, Jul 02, 2026 at 11:58:06AM +0100, Leonardo Bras wrote:
> 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 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.
>
> 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.
Yeah, and I believe using guard() is encouraged, since it's less error
prone.
>
> >
> > >
> > > >
> > > > - 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?
Correct.
>
> 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.
In kvm_ptdump_guest_nested_show() I acquire the mmu_lock, therefore
"initially finds N but printing N-1" won't happen, the whole operation
of looking at the nested mmu array and printing them out is atomic.
>
> 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.
Yes, that's one of the problems not taking the lock can introduce.
Thanks,
Wei-Lin Chang
>
> >
> > >
> > > >
> > > > 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
More information about the linux-arm-kernel
mailing list