linux-kernel - Re: [PATCH v2 06/13] KVM: arm64: Add support for KVM_MEM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aDebZD1Kmmg15zs7@google.com>
Date: Wed, 28 May 2025 16:25:24 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: amoorthy@...gle.com, corbet@....net, dmatlack@...gle.com, 
	kalyazin@...zon.com, kvm@...r.kernel.org, kvmarm@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, maz@...nel.org, oliver.upton@...ux.dev, 
	pbonzini@...hat.com, peterx@...hat.com, pgonda@...gle.com, 
	wei.w.wang@...el.com, yan.y.zhao@...el.com
Subject: Re: [PATCH v2 06/13] KVM: arm64: Add support for KVM_MEM_USERFAULT

On Wed, May 28, 2025, James Houghton wrote:
> On Wed, May 28, 2025 at 1:30 PM Sean Christopherson <seanjc@...gle.com> wrote:
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index c5d21bcfa3ed4..f1db3f7742b28 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -2127,15 +2131,23 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  				   const struct kvm_memory_slot *new,
>  				   enum kvm_mr_change change)
>  {
> -	bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES;
> +	u32 old_flags = old ? old->flags : 0;
> +	u32 new_flags = new ? new->flags : 0;
> +
> +	/*
> +	 * If only changing flags, nothing to do if not toggling
> +	 * dirty logging.
> +	 */
> +	if (change == KVM_MR_FLAGS_ONLY &&
> +	    !((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES))
> +		return;
>  
>  	/*
>  	 * At this point memslot has been committed and there is an
>  	 * allocated dirty_bitmap[], dirty pages will be tracked while the
>  	 * memory slot is write protected.
>  	 */
> -	if (log_dirty_pages) {
> -
> +	if (new_flags & KVM_MEM_LOG_DIRTY_PAGES) {
>  		if (change == KVM_MR_DELETE)
>  			return;
>  
> 
> So we need to bail out early if we are enabling KVM_MEM_USERFAULT but
> KVM_MEM_LOG_DIRTY_PAGES is already enabled, otherwise we'll be
> write-protecting a bunch of PTEs that we don't need or want to WP.
> 
> When *disabling* KVM_MEM_USERFAULT, we definitely don't want to WP
> things, as we aren't going to get the unmap afterwards anyway.
> 
> So the check we started with handles this:
> > > > > +       u32 old_flags = old ? old->flags : 0;
> > > > > +       u32 new_flags = new ? new->flags : 0;
> > > > > +
> > > > > +       /* Nothing to do if not toggling dirty logging. */
> > > > > +       if (!((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES))
> > > > > +               return;
> 
> So why also check for `change == KVM_MR_FLAGS_ONLY` as well? Everything I just
> said doesn't really apply when the memslot is being created, moved, or
> destroyed. Otherwise, consider the case where we never enable dirty logging:
> 
>  - Memslot deletion would be totally broken; we'll see that
>    KVM_MEM_LOG_DIRTY_PAGES is not getting toggled and then bail out, skipping
>    some freeing.

No, because @new and thus new_flags will be 0.  If dirty logging wasn't enabled,
then there's nothing to be done.

>  - Memslot creation would be broken in a similar way; we'll skip a bunch of
>    setup work.

No, because @old and thus old_flags will be 0.  If dirty logging isn't being
enabled, then there's nothing to be done.

>  - For memslot moving, the only case that we could possibly be leaving
>    KVM_MEM_LOG_DIRTY_PAGES set without the change being KVM_MR_FLAGS_ONLY,
>    I think we still need to do the split and WP stuff.

No, because KVM invokes kvm_arch_flush_shadow_memslot() on the memslot and marks
it invalid prior to installing the new, moved memslot.  See kvm_invalidate_memslot().

So I'm still not seeing what's buggy.