All of lore.kernel.org
 help / color / mirror / Atom feed
From: Will Deacon <will@kernel.org>
To: kvmarm@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Will Deacon <will@kernel.org>,
	Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Ricardo Koller <ricarkol@google.com>
Subject: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()
Date: Mon, 12 Feb 2024 19:30:52 +0000	[thread overview]
Message-ID: <20240212193052.27765-1-will@kernel.org> (raw)

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

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);
 	}
 
@@ -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;
 	}
 
-- 
2.43.0.687.g38aa6559b0-goog


WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will@kernel.org>
To: kvmarm@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Will Deacon <will@kernel.org>,
	Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Ricardo Koller <ricarkol@google.com>
Subject: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()
Date: Mon, 12 Feb 2024 19:30:52 +0000	[thread overview]
Message-ID: <20240212193052.27765-1-will@kernel.org> (raw)

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

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);
 	}
 
@@ -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;
 	}
 
-- 
2.43.0.687.g38aa6559b0-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

             reply	other threads:[~2024-02-12 19:30 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-12 19:30 Will Deacon [this message]
2024-02-12 19:30 ` [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked() Will Deacon
2024-02-12 20:14 ` Oliver Upton
2024-02-12 20:14   ` Oliver Upton
2024-02-12 20:19   ` Oliver Upton
2024-02-12 20:19     ` Oliver Upton
2024-02-13 11:12   ` Marc Zyngier
2024-02-13 11:12     ` Marc Zyngier
2024-02-13 12:12     ` Will Deacon
2024-02-13 12:12       ` Will Deacon
2024-02-13 16:29     ` Oliver Upton
2024-02-13 16:29       ` Oliver Upton
2024-02-13 16:52       ` Marc Zyngier
2024-02-13 16:52         ` Marc Zyngier
2024-02-13 19:25 ` Marc Zyngier
2024-02-13 19:25   ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240212193052.27765-1-will@kernel.org \
    --to=will@kernel.org \
    --cc=kvmarm@lists.linux.dev \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=oliver.upton@linux.dev \
    --cc=ricarkol@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.