| Message ID | 20240212193052.27765-1-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked() | expand |
On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote: > 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); > } 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(). > @@ -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.
On Mon, Feb 12, 2024 at 08:14:41PM +0000, Oliver Upton wrote: > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote: > > 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); > > } > > 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(). > > > @@ -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. That is to say, a race with a *HW* update to the PTE that sets AF. > There shouldn't be a race with any software walkers, as we > hold the MMU lock for write here. > > -- > Thanks, > Oliver
On Mon, 12 Feb 2024 20:14:37 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote: > > 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()") I'd blame it on the last commit, as we really ought to have it if we have the others. > > > > 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); > > } > > 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(). Can't you construct a case where an allocation fails during the walk (memcache empty), and we end up on this exact path? > > > @@ -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.
On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote: > On Mon, 12 Feb 2024 20:14:37 +0000, > Oliver Upton <oliver.upton@linux.dev> wrote: > > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote: > > > 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. Yes, that's probably the best approach if you're adding a Fixes tag. > > > @@ -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. 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
On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote: > On Mon, 12 Feb 2024 20:14:37 +0000, > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote: > > > 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()") > > I'd blame it on the last commit, as we really ought to have it if we > have the others. > > > > > > > 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); > > > } > > > > 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(). > > 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 :) > > > > > @@ -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. Agreed, happy with the way this looks and should've added: Reviewed-by: Oliver Upton <oliver.upton@linux.dev> the first time around.
On Tue, 13 Feb 2024 16:29:42 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote: > > On Mon, 12 Feb 2024 20:14:37 +0000, > > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > > > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote: > > > > 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()") > > > > I'd blame it on the last commit, as we really ought to have it if we > > have the others. > > > > > > > > > > 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); > > > > } > > > > > > 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(). > > > > 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 :) Yeah, that's probably very unlikely to hit given the current conditions. > > > > > > > > @@ -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. > > Agreed, happy with the way this looks and should've added: > > Reviewed-by: Oliver Upton <oliver.upton@linux.dev> > > the first time around. Thanks for that. I'll queue that shortly and send (another) PR. M.
On Mon, 12 Feb 2024 19:30:52 +0000, Will Deacon wrote: > 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.
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; }
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(-)