KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

RFC PATCH Applied Will Deacon 3 participants 8 messages Feb 12, 2024 – Feb 13, 2024

Subsystems (2)

arm64 KVM arm64

lore

Share & tools

lore

Will Deacon

author maintainer

2024-02-12 19:30 UTC #0 lore raw

RFC PATCH 1/1 KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

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 <[email protected]>

Cc Oliver Upton <[email protected]>

Cc Ricardo Koller <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

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()")

are actually ok in isolation. Hrm. Please tell me I'm wrong?

 arch/arm64/kvm/hyp/pgtable.c | 2 --
 1 file changed, 2 deletions(-)

arch/arm64/kvm/hyp/pgtable.c +0 -2

index c651df904fe3..ab9d05fcf98b 100644
@@ -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);
 	}

@@ -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;
 	}

Signature · 1 line

2.43.0.687.g38aa6559b0-goog

Oliver Upton

2024-02-12 20:14 UTC #1 lore raw

Quoted 30 lines · Will Deacon

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 <[email protected]>
Cc Oliver Upton <[email protected]>
Cc Ricardo Koller <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
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()")
are actually ok in isolation. Hrm. Please tell me I'm wrong?
 arch/arm64/kvm/hyp/pgtable.c | 2 --
 1 file changed, 2 deletions(-)
arch/arm64/kvm/hyp/pgtable.c +0 -0
index c651df904fe3..ab9d05fcf98b 100644

arch/arm64/kvm/hyp/pgtable.c kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, · line 1423

Quoted 6 lines

arch/arm64/kvm/hyp/pgtable.c +0 -1

@@ -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().

arch/arm64/kvm/hyp/pgtable.c static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, · line 1505

Quoted 7 lines

arch/arm64/kvm/hyp/pgtable.c +0 -1

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

Signature · 2 lines

Thanks,
Oliver

Oliver Upton

2024-02-12 20:19 UTC #2 lore raw

Quoted 49 lines · Oliver Upton

Quoted 30 lines · Will Deacon
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 <[email protected]>
Cc Oliver Upton <[email protected]>
Cc Ricardo Koller <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
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()")
are actually ok in isolation. Hrm. Please tell me I'm wrong?
 arch/arm64/kvm/hyp/pgtable.c | 2 --
 1 file changed, 2 deletions(-)
arch/arm64/kvm/hyp/pgtable.c +0 -0
index c651df904fe3..ab9d05fcf98b 100644
arch/arm64/kvm/hyp/pgtable.c kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, · line 1423
Quoted 6 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
@@ -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().
Quoted 2 lines
arch/arm64/kvm/hyp/pgtable.c +0 -0
@@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
 

Quoted 9 lines

arch/arm64/kvm/hyp/pgtable.c static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, · line 1505
Quoted 5 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
 	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.

That is to say, a race with a *HW* update to the PTE that sets AF.

Quoted 6 lines

There shouldn't be a race with any software walkers, as we hold the MMU lock for write here.
Signature · 2 lines
Thanks,
Oliver

Signature · 2 lines

Thanks,
Oliver

Marc Zyngier

maintainer

2024-02-13 11:12 UTC #3 lore raw

In reply to Oliver Upton

Quoted 14 lines · Oliver Upton

Quoted 12 lines · Will Deacon
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 <[email protected]>
Cc Oliver Upton <[email protected]>
Cc Ricardo Koller <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
Hi folks,

Quoted 7 lines

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

Quoted 23 lines

Quoted 11 lines
are actually ok in isolation. Hrm. Please tell me I'm wrong?
 arch/arm64/kvm/hyp/pgtable.c | 2 --
 1 file changed, 2 deletions(-)
arch/arm64/kvm/hyp/pgtable.c +0 -0
index c651df904fe3..ab9d05fcf98b 100644
arch/arm64/kvm/hyp/pgtable.c kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, · line 1423
Quoted 6 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
@@ -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.

Quoted 3 lines

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?

Quoted 13 lines

arch/arm64/kvm/hyp/pgtable.c static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, · line 1505
Quoted 8 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
@@ -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.

Thanks,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Will Deacon

author maintainer

2024-02-13 12:12 UTC #4 lore raw

Quoted 7 lines · Marc Zyngier

Quoted 5 lines · Oliver Upton
Quoted 4 lines · Will Deacon
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:

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
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.

Yes, that's probably the best approach if you're adding a Fixes tag.

Quoted 13 lines

Quoted 12 lines
arch/arm64/kvm/hyp/pgtable.c static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, · line 1505
Quoted 7 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
@@ -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.

Quoted 5 lines

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.

Fine by me! Even though I found it by inspection, I have taken the patch for a spin to check that I (somehow) didn't break something else.

Will

Oliver Upton

Reviewed-by

2024-02-13 16:29 UTC #5 lore raw

In reply to Marc Zyngier

Quoted 50 lines · Marc Zyngier

Quoted 14 lines · Oliver Upton
Quoted 12 lines · Will Deacon
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 <[email protected]>
Cc Oliver Upton <[email protected]>
Cc Ricardo Koller <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
Hi folks,
Quoted 7 lines
Quoted 7 lines
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.
Quoted 23 lines
Quoted 11 lines
are actually ok in isolation. Hrm. Please tell me I'm wrong?
 arch/arm64/kvm/hyp/pgtable.c | 2 --
 1 file changed, 2 deletions(-)
arch/arm64/kvm/hyp/pgtable.c +0 -0
index c651df904fe3..ab9d05fcf98b 100644
arch/arm64/kvm/hyp/pgtable.c kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, · line 1423
Quoted 6 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
@@ -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.

Quoted 6 lines

Quoted 3 lines
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 :)

Quoted 14 lines

Quoted 13 lines
arch/arm64/kvm/hyp/pgtable.c static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, · line 1505
Quoted 8 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
@@ -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.

Quoted 5 lines

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 <[email protected]>

the first time around.

Signature · 2 lines

Thanks,
Oliver

Marc Zyngier

maintainer

2024-02-13 16:52 UTC #6 lore raw

Quoted 59 lines · Oliver Upton

Quoted 50 lines · Marc Zyngier
Quoted 14 lines · Oliver Upton
Quoted 12 lines · Will Deacon
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 <[email protected]>
Cc Oliver Upton <[email protected]>
Cc Ricardo Koller <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
Hi folks,
Quoted 7 lines
Quoted 7 lines
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.
Quoted 23 lines
Quoted 11 lines
are actually ok in isolation. Hrm. Please tell me I'm wrong?
 arch/arm64/kvm/hyp/pgtable.c | 2 --
 1 file changed, 2 deletions(-)
arch/arm64/kvm/hyp/pgtable.c +0 -0
index c651df904fe3..ab9d05fcf98b 100644
arch/arm64/kvm/hyp/pgtable.c kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt, · line 1423
Quoted 6 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
@@ -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.
Quoted 6 lines
Quoted 3 lines
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?

Quoted 8 lines

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 :)

Yeah, that's probably very unlikely to hit given the current conditions.

Quoted 21 lines

Quoted 15 lines
Quoted 4 lines
Quoted 4 lines
arch/arm64/kvm/hyp/pgtable.c +0 -0
@@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
 
Quoted 10 lines
arch/arm64/kvm/hyp/pgtable.c static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx, · line 1505
Quoted 5 lines
arch/arm64/kvm/hyp/pgtable.c +0 -1
 	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.
Quoted 5 lines
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.

Quoted 5 lines

Agreed, happy with the way this looks and should've added:
Reviewed-by Oliver Upton <[email protected]>
the first time around.

Thanks for that. I'll queue that shortly and send (another) PR.

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Marc Zyngier

maintainer Applied

2024-02-13 19:25 UTC #7 lore raw

In reply to Will Deacon

Quoted 3 lines · Will Deacon

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.

Applied to fixes, thanks!

[1/1] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

      commit: c60d847be7b8e69e419e02a2b3d19c2842a3c35d

Cheers,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.