Re: [RFC PATCH v2 10/11] samples/kvm: Add a memory provider sample

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] kvm: misaligned huge page PFN causes host memory corruption
- [High] mm: bitmap integer truncation leads to out-of-bounds write
--

commit 361e4f16cd6774e9f8e63152565759849581de53
Author: Fred Griffoul <fgriffo@amazon.co.uk>

samples/kvm: Add a memory provider sample

This patch adds a sample memory provider for guest_memfd and iommufd. It lends a
physical memory region, either given by addr/len parameters or allocated via
alloc_contig_pages(), to guest VMs and moves pages between children.

> diff --git a/samples/kvm/mem_provider_sample.c b/samples/kvm/mem_provider_sample.c
> index 0000000000000..8180959ab78ff 100644
> --- /dev/null
> +++ b/samples/kvm/mem_provider_sample.c
> @@ -0,0 +1,855 @@
[ ... ]
> +static int mps_mp_get_page(struct mem_provider_file *mpf, pgoff_t index,
> +			   unsigned long *pfn, int *max_order, u32 *attrs)
> +{
> +	struct mps_info *info = container_of(mpf, struct mps_info, mpf);
> +	unsigned long run;
> +
> +	if (index >= info->npages)
> +		return -EINVAL;
> +
> +	if (!mps_page_present(info, index))
> +		return -EFAULT;
> +
> +	/* The aligned block around @index must be one run. */
> +	run = mps_run(info, index);
> +	while (*max_order) {
> +		unsigned long first = ALIGN_DOWN(index, 1UL << *max_order);
> +
> +		if (first + (1UL << *max_order) <= index + run &&
> +		    (first == index || mps_page_present(info, first)) &&
> +		    mps_run(info, first) >= 1UL << *max_order)
> +			break;
> +		(*max_order)--;
> +	}

[Severity: Critical]
Are we missing a hardware PFN alignment check for huge pages?

The loop validates that the internal state is consistent for a block, but
doesn't verify if info->base_pfn + first is naturally aligned to the huge
page boundary (1UL << *max_order).

Because alloc_contig_pages() or the addr module parameter do not guarantee
alignment to arbitrary huge page boundaries, KVM or the hardware MMU could
force alignment by masking the lower bits of the PFN. Would this result in
the guest mapping memory before the intended base_pfn and accessing unowned
host memory?

[ ... ]
> +static void mps_set_readonly(struct mps_info *info, unsigned long start,
> +			     unsigned long end, bool readonly)
> +{
> +	mutex_lock(&info->lock);
> +	if (readonly)
> +		bitmap_set(info->readonly, start, end - start);
> +	else
> +		bitmap_clear(info->readonly, start, end - start);

[Severity: High]
Can start and end - start silently truncate here?

These parameters are unsigned long, but bitmap_set() and bitmap_clear()
expect unsigned int for the start and nbits arguments. If the region size
is extremely large, this could result in mapping into a truncated bitmap
footprint and corrupting adjacent kernel heap memory.

> +	mps_revoke(info, start, end);
> +	mutex_unlock(&info->lock);
> +}
[ ... ]
> +static int mps_new_file(unsigned long base_pfn, unsigned long npages,
> +			u32 flags, struct mps_info **infop,
> +			 struct file **filep)
> +{
> +	struct mps_info *info;
> +	struct file *file;
> +	int fd, ret;
> +
> +	info = kzalloc_obj(*info);
> +	if (!info)
> +		return -ENOMEM;
> +	mem_provider_file_init(&info->mpf);
> +	info->base_pfn = base_pfn;
> +	info->npages = npages;
> +	info->mmap_capable = flags & MPS_FLAG_MMAP_CAPABLE;
> +	info->absent = bitmap_zalloc(npages, GFP_KERNEL);
> +	info->readonly = bitmap_zalloc(npages, GFP_KERNEL);

[Severity: High]
Will the allocation size be truncated here?

The npages parameter is an unsigned long derived from the user-controlled
setup.size (via ioctl) or the len module parameter. Because bitmap_zalloc()
accepts an unsigned int for nbits, any npages value exceeding UINT_MAX (e.g.,
a region > 16 TB) will be silently truncated, causing massively undersized
bitmaps to be allocated.

> +	if (!info->absent || !info->readonly) {
> +		ret = -ENOMEM;
> +		goto err_free;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014093733.90312-1-griffoul@gmail.com?part=10