Re: [RFC PATCH v2 06/10] kvm: guest_memfd: Add support for freezing and unfreezing mappings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A malicious tenant can inflict a cross-tenant denial-of-service by holding the global `kvm_gmem_freeze_srcu` read lock during massive, unbounded `fallocate()` operations, blocking `kvm_gmem_freeze()` for other VMs.
--

commit 5ee5700d9538d9bd5f29f00de6d11fcc22d28ecc
Author: Tarun Sahu <tarunsahu@google.com>

kvm: guest_memfd: Add support for freezing and unfreezing mappings

This commit introduces freezing and unfreezing for guest_memfd mappings to
prevent fallocate and page fault allocations during preservation. It uses a
global SRCU structure to synchronize read-side operations with
kvm_gmem_freeze().

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 6740ae2bf948d..b94639cdf3126 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -273,16 +284,30 @@ static long kvm_gmem_allocate(struct inode *inode, loff_t offset, loff_t len)
>  static long kvm_gmem_fallocate(struct file *file, int mode, loff_t offset,
>  			       loff_t len)
>  {
> +	struct inode *inode = file_inode(file);
>  	int ret;
> +	int idx;
>  
> -	if (!(mode & FALLOC_FL_KEEP_SIZE))
> -		return -EOPNOTSUPP;
> +	idx = srcu_read_lock(&kvm_gmem_freeze_srcu);
> +	if (kvm_gmem_is_frozen(inode)) {
> +		srcu_read_unlock(&kvm_gmem_freeze_srcu, idx);
> +		return -EPERM;
> +	}
>  
[ ... ]
> +	if (!PAGE_ALIGNED(offset) || !PAGE_ALIGNED(len)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	if (mode & FALLOC_FL_PUNCH_HOLE)
>  		ret = kvm_gmem_punch_hole(file_inode(file), offset, len);
>  	else
>  		ret = kvm_gmem_allocate(file_inode(file), offset, len);

[Severity: High]
Does this code allow a malicious tenant to inflict a cross-tenant denial of
service?

The srcu_read_lock() is acquired at the beginning of kvm_gmem_fallocate() and
held across the entire kvm_gmem_punch_hole() or kvm_gmem_allocate() execution.
Because a guest_memfd can be terabytes in size, these operations could iterate
over millions of pages and take a long time to complete.

During this entire time, the global SRCU read lock is held uninterruptibly
(yielding via cond_resched() in kvm_gmem_allocate() does not drop SRCU).

>  
>  	if (!ret)
>  		file_modified(file);
> +
> +out:
> +	srcu_read_unlock(&kvm_gmem_freeze_srcu, idx);
>  	return ret;
>  }
[ ... ]
> @@ -1014,5 +1053,61 @@ void kvm_gmem_exit(void)
[ ... ]
> +void kvm_gmem_freeze(struct inode *inode, bool freeze)
> +{
> +	u64 flags = READ_ONCE(GMEM_I(inode)->flags);
> +
> +	if (freeze)
> +		flags |= GUEST_MEMFD_F_MAPPING_FROZEN;
> +	else
> +		flags &= ~GUEST_MEMFD_F_MAPPING_FROZEN;
> +
> +	WRITE_ONCE(GMEM_I(inode)->flags, flags);
> +
> +	if (freeze)
> +		synchronize_srcu(&kvm_gmem_freeze_srcu);

[Severity: High]
Can this synchronize_srcu() block indefinitely waiting on another tenant?

Since the SRCU is global, calling kvm_gmem_freeze() on a completely unrelated
VM will invoke synchronize_srcu(&kvm_gmem_freeze_srcu). This will wait for the
first tenant's massive fallocate() to finish.

The loop in kvm_gmem_allocate() checks kvm_gmem_is_frozen(inode) on its own
inode, so it will not abort when a different inode is frozen. This effectively
allows an unprivileged tenant to stall live updates and preservation
system-wide.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780676742.git.tarunsahu@google.com?part=6