Re: [PATCH] KVM: Reject wrapped offset in kvm_reset_dirty_gfn()

[Paolo Bonzini <pbonzini@redhat.com>]

On 5/12/26 08:07, Aaron Sacks wrote:
> kvm_reset_dirty_gfn() guards the gfn range with
> 
> 	if (!memslot || (offset + __fls(mask)) >= memslot->npages)
> 		return;
> 
> but offset is u64 and the addition is unchecked.  The check can be
> silently bypassed by a u64 wrap.
> 
> The dirty ring backing those entries is MAP_SHARED at
> KVM_DIRTY_LOG_PAGE_OFFSET of the vcpu fd, so the VMM can rewrite the
> slot and offset fields of any entry between when the kernel pushes
> them and when KVM_RESET_DIRTY_RINGS consumes them.  On reset,
> kvm_dirty_ring_reset() re-reads the values via READ_ONCE() and feeds
> them straight back into this check; only the flags handshake is
> treated as the handover, the slot/offset payload is taken on trust.
> 
> Crafting two entries
> 
> 	entry[i].offset   = 0xffffffffffffffc1
> 	entry[i+1].offset = 0
> 
> makes the coalescing loop in kvm_dirty_ring_reset() compute
> 
> 	delta = (s64)(0 - 0xffffffffffffffc1) = 63
> 
> which falls in [0, BITS_PER_LONG), so it folds entry[i+1] into the
> existing mask by setting bit 63.  The trailing kvm_reset_dirty_gfn()
> call then sees offset = 0xffffffffffffffc1 and __fls(mask) = 63;
> the sum is 0 in u64 and the bounds check passes.
> 
> That offset propagates into kvm_arch_mmu_enable_log_dirty_pt_masked()
> unchanged.  On the legacy MMU path -- kvm_memslots_have_rmaps() ==
> true, i.e. shadow paging, any VM that has allocated shadow roots, or
> a write-tracked slot -- it reaches gfn_to_rmap(), which indexes
> slot->arch.rmap[0][] with a near-U64_MAX gfn.  That is an
> out-of-bounds load of a kvm_rmap_head, followed by a conditional
> clear of PT_WRITABLE_MASK in whatever the loaded pointer points at.
> The path is reachable from any process holding /dev/kvm.
> 
> Range-check offset on its own first, so the addition cannot wrap.
> memslot->npages is bounded well below U64_MAX, so once offset <
> npages holds, offset + __fls(mask) (with __fls(mask) < BITS_PER_LONG)
> stays in range.
> 
> Fixes: fb04a1eddb1a ("KVM: X86: Implement ring-based dirty memory tracking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aaron Sacks <contact@xchglabs.com>
> ---
> Hi Willy,
> 
> Thanks for the review.  Re-sending this one as a proper patch and
> addressing your points below.  I will reply separately to the
> other three reports per your guidance there.
> 
>> Please could you check on an up-to-date kernel? 6.13.7 is long
>> dead (1 year) and thousands of fixes were merged since.
> 
> Re-verified on v7.0.6 (current stable). The buggy bounds check
> in virt/kvm/dirty_ring.c is unchanged:
> 
>      if (!memslot || (offset + __fls(mask)) >= memslot->npages)
>          return;
> 
> Same PoC still produces the oops:
> 
>      Comm: poc Not tainted 7.0.6 #1
>      BUG: unable to handle page fault for address: ffffa9bbc002ce08
>      RIP: 0010:rmap_write_protect+0x6/0xf0
>      RAX: ffffffffffffffc1   RBX: 8000000000000001
>      Call Trace:
>       kvm_arch_mmu_enable_log_dirty_pt_masked+0x145/0x210
>       kvm_reset_dirty_gfn+0xcd/0x100
>       kvm_dirty_ring_reset+0x12c/0x1f0
>       kvm_vm_ioctl+0xb41/0x10b0
>      Kernel panic - not syncing: Fatal exception
> 
> RAX is the crafted wrapped offset; RBX is the coalesced mask with
> bit 63 set (__fls == 63).  Both match the values planted in the
> ring entries from userspace, so the wrapped sum still passes the
> existing bounds check on v7.0.6.
> 
>> Please also wrap long lines
> 
> Done -- this reply and the patch are at <=72 cols.
> 
>> Was all of this generated by an LLM?
> 
> The reports were generated by an LLM yes. But the analysis, PoC
> and patch suggestions are mine. Happy to provide the PoC source,
> kernel .config, or additional artifacts.
> 
>> Could you please turn this one into a real patch
> 
> Patch follows. Applies clean on v7.0.6; with it applied, the
> same trigger no longer oopses (offset is rejected by the added
> offset >= npages check before the addition can wrap).

Applied, thanks.

Paolo