Re: [PATCH RFC v3 54/88] KVM: guest_memfd: Support read and write operations and provider callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] KVM: guest_memfd: corrupt *ppos on copy_to_user() fault in
  kvm_gmem_rw()
- [Low] KVM: guest_memfd: missing kernel-doc for provider read and write
  callbacks
--

commit e75034915fb7c05a994b62162e0f8d89c6caa18a
Author: Ackerley Tng <ackerleytng@google.com>

KVM: guest_memfd: Support read and write operations and provider callbacks

This commit adds read and write file operations for guest_memfd descriptors,
using a stack bounce buffer to forward shared memory reads and writes to
provider callbacks.

> diff --git a/include/linux/guest_memfd.h b/include/linux/guest_memfd.h
> index cd634576c9af8..c85594b58112a 100644
> --- a/include/linux/guest_memfd.h
> +++ b/include/linux/guest_memfd.h
> @@ -113,6 +113,10 @@ struct guest_memfd_provider_operations {
>  				unsigned long *pfn, size_t *nr_pages);
>  	void (*put_pfn_range)(void *provider, pgoff_t index, size_t nr_pages);
>  	int (*mmap)(void *provider, struct vm_area_struct *vma);
> +	ssize_t (*read)(void *provider, void *buf, size_t count,
> +			loff_t *ppos);
> +	ssize_t (*write)(void *provider, const void *buf, size_t count,
> +			 loff_t *ppos);
>  	int (*check_set_attributes)(void *provider, pgoff_t index,
>  				    size_t nr_pages, u64 attributes);
>  };

[Severity: Low]
struct guest_memfd_provider_operations has a kernel-doc comment block above
it, so adding read and write without @read and @write descriptions triggers
scripts/kernel-doc warnings.

Could the kernel-doc comment also document that these callbacks run under
the shared filemap invalidate lock (so they must not take locks held across
kvm_gmem_revoke()) and are expected to advance *ppos by the number of bytes
transferred?

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 61a3ff7fcd84c..385b517b49ed5 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -1204,7 +1239,86 @@ static long kvm_gmem_ioctl(struct file *file, unsigned int ioctl,
>  	}
>  }
>  
> +static ssize_t kvm_gmem_rw(struct file *file, void __user *buf, size_t count,
> +			   loff_t *ppos, bool is_write)
> +{

[ ... ]

> +	while (count) {
> +		u8 bounce[256];
> +		size_t chunk = min_t(size_t, count, sizeof(bounce));
> +		pgoff_t start = *ppos >> PAGE_SHIFT;
> +		pgoff_t end = (*ppos + chunk - 1) >> PAGE_SHIFT;
> +		pgoff_t nr_pages = end - start + 1;
> +		ssize_t ret;
> +
> +		if (is_write) {
> +			if (copy_from_user(bounce, (const char __user *)buf + total_done, chunk))
> +				return total_done ?: -EFAULT;
> +		}
> +
> +		filemap_invalidate_lock_shared(inode->i_mapping);
> +
> +		if (!kvm_gmem_range_has_attributes(inode, start, nr_pages, 0)) {
> +			filemap_invalidate_unlock_shared(inode->i_mapping);
> +			return total_done ?: -EFAULT;
> +		}
> +
> +		if (is_write)
> +			ret = gmem_provider_write(gi, bounce, chunk, ppos);
> +		else
> +			ret = gmem_provider_read(gi, bounce, chunk, ppos);
> +
> +		filemap_invalidate_unlock_shared(inode->i_mapping);
> +
> +		if (ret < 0)
> +			return total_done ?: ret;
> +
> +		if (!ret)
> +			break;
> +
> +		if (!is_write) {
> +			if (copy_to_user((char __user *)buf + total_done, bounce, ret))
> +				return total_done ?: -EFAULT;
> +		}

[Severity: Medium]
Does a copy_to_user() fault here leave *ppos advanced past the unread
chunk?

gmem_provider_read() updates *ppos by ret before copy_to_user() runs.  If a
read spanning multiple 256-byte chunks faults in copy_to_user() after an
earlier chunk succeeded (total_done > 0), kvm_gmem_rw() returns total_done
while *ppos has already been incremented by total_done + ret, so ksys_read()
commits the over-advanced offset to file->f_pos and the next sequential read
silently skips ret bytes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=54