Re: [PATCH v2 4/9] KVM: s390: Fix dirty marking in adapter_indicators_set*()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Replacing set_page_dirty_lock() with set_page_dirty() on GUP-pinned pages introduces a Use-After-Free vulnerability due to a race with file truncation.
- [High] Ordering mark_page_dirty() before memory modification creates a race condition with dirty log harvesting, risking lost updates during live migration.
--

--- Patch [4]: [PATCH v2 4/9] KVM: s390: Fix dirty marking in adapter_indicators_set*() ---
commit 8bb6adeda8416869b198540515723990915f8390
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix dirty marking in adapter_indicators_set*()

This commit updates dirty page marking for adapter indicators in KVM s390.
It adds missing calls to set_page_dirty() and mark_page_dirty() in the fast
path and reorganizes the slow path, replacing set_page_dirty_lock() with
set_page_dirty() for pinned pages.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 608ff95d53bac..e79f636c7544d 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
[ ... ]
> @@ -2997,21 +2999,20 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		ind_page = pin_map_page(kvm, adapter_int->ind_addr, 0);
>  		if (!ind_page)
>  			return -1;
> -		idx = srcu_read_lock(&kvm->srcu);
>  		map = page_address(ind_page);
>  		bit = get_ind_bit(adapter_int->ind_addr,
>  				  adapter_int->ind_offset, adapter->swap);
>  		set_bit(bit, map);
> -		mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
> -		set_page_dirty_lock(ind_page);
> -		srcu_read_unlock(&kvm->srcu, idx);
> +		set_page_dirty(ind_page);

[Severity: High]
Is it safe to replace set_page_dirty_lock() with set_page_dirty() here?

For short-term GUP-pinned memory like this ind_page, the memory can be backed
by any filesystem. Without the folio lock, a concurrent file truncation could
unmap the folio and free the mapping.

If set_page_dirty() reads folio->mapping concurrently, could it dereference
freed memory when calling mapping->a_ops->dirty_folio()?

>  		unpin_user_page(ind_page);
>  	} else {
>  		map = page_address(ind_info->page);
>  		bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
>  		set_bit(bit, map);
> +		set_page_dirty(ind_info->page);
>  		spin_unlock_irqrestore(&adapter->maps_lock, flags);
>  	}
> +	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
>  
>  	spin_lock_irqsave(&adapter->maps_lock, flags);
>  	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> @@ -3020,22 +3021,21 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
>  		if (WARN_ON_ONCE(!summary_page))
>  			return -1;
> -		idx = srcu_read_lock(&kvm->srcu);
>  		map = page_address(summary_page);
>  		bit = get_ind_bit(adapter_int->summary_addr,
>  				  adapter_int->summary_offset, adapter->swap);
>  		summary_set = test_and_set_bit(bit, map);
> -		mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
> -		set_page_dirty_lock(summary_page);
> -		srcu_read_unlock(&kvm->srcu, idx);
> +		set_page_dirty(summary_page);

[Severity: High]
Similarly, could calling set_page_dirty() instead of set_page_dirty_lock() on
the GUP-pinned summary_page lead to a use-after-free if the file is truncated
concurrently?

>  		unpin_user_page(summary_page);
>  	} else {
>  		map = page_address(summary_info->page);
>  		bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
>  				  adapter->swap);
>  		summary_set = test_and_set_bit(bit, map);
> +		set_page_dirty(summary_info->page);
>  		spin_unlock_irqrestore(&adapter->maps_lock, flags);
>  	}
> +	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->summary_gaddr));
>  
>  	return summary_set ? 0 : 1;
>  }
> @@ -3045,26 +3045,30 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
>  				       struct kvm_s390_adapter_int *adapter_int,
>  				       int setbit)
>  {
[ ... ]
>  	map = page_address(ind_info->page);
>  	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> -	if (setbit)
> +	if (setbit) {
> +		mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
> +		set_page_dirty(ind_info->page);
>  		set_bit(bit, map);
> +	}

[Severity: High]
Does calling mark_page_dirty() before actually modifying the memory with
set_bit() create a race condition with dirty log harvesting?

If userspace harvests the KVM dirty log between mark_page_dirty() and
set_bit(), it might clear the dirty bit in KVM's tracking and read the old
unmodified page.

When set_bit() then executes, the page would no longer be marked dirty, and
the change wouldn't be migrated to the destination.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4