Re: [PATCH v2 2/5] KVM: guest_memfd: Fix possible signed integer overflow

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> For shortlogs (and changeloges), when possible, describe the _change_ itself, not
> its impact is.	Sometimes "Fix xyz" is the best shortlog, e.g. when fixing build
> failures, but here, I would go with:
>
>   KVM: guest_memfd: Treat memslot binding offset+size as unsigned values
>
> for two reasons.  First, it provides a lot more context for future readers, versus
> "Fix possible signed integer overflow" which doesn't even capture what flow is
> affected, how the overflow is being fixed, etc.  Second, if the fix is wrong,
> incomplete, etc., we don't end up with a follow-up patch that start with "Really
> fix ...".
>

Thanks for explaining!

> Oh, actually, three reasons.  This doesn't only affect the overflow check.  The
> check on a negative offset is flawed, as it means KVM would incorrectly reject
> bindings with (comically) large offsets.
>

Makes sense.

> LOL, four.  There is no bug.  The size of the memslot is ((1UL << 31) - 1)
> pages, i.e. 0x7FF_FFFFF000:
>
> 	if (id < KVM_USER_MEM_SLOTS &&
> 	    (mem->memory_size >> PAGE_SHIFT) > KVM_MEM_MAX_NR_PAGES)
> 		return -EINVAL;
>
> and so "loff_t size" can never be negative.
>

I think the bug was that the sum of offset + size in kvm_gmem_bind()
when interpreted as signed integers could be smaller than
i_size_read(inode) and allow binding.

So IIUC even if size is small (and not negative), nothing catches a
large enough offset where offset + size (interpreted as unsigned
integers) doesn't overflow, but offset + size (interpreted as signed
integers) overflows.

> As for the offset, the negative check is intentional, because KVM_CREATE_GUEST_MEMFD
> takes loff_t for the size, and so an offset that is negative would also be larger
> than the size of the file.
>
> I still think it's worth taking unsigned values, because teasing out all of that
> information wasn't exactly easy.
>

Yup it's still easier this way, and your proposed shortlog is good.

> On Fri, May 22, 2026, Ackerley Tng wrote:
>> From: Sean Christopherson <seanjc@xxxxxxxxxx>
>>
>> The caller, kvm_set_memory_region(), checks for an overflow in an unsigned
>> u64 guest_memfd_offset. When guest_memfd_offset is passed to kvm_gmem_bind,
>> it is cast into a signed 64-bit integer.
>>
>> Hence, a large 64-bit offset could result in a negative loff_t, which could
>> result in the overflow checks failing.
>>
>> Make kvm_gmem_bind() take u64 instead of loff_t to consistently deal with
>> unsigned values to avoid this issue.
>>
>> Fixes: a7800aa80ea4d ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
>> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>> [Use size_t for size instead of u64]
>
> Why?  Oh, right, because kvm_memory_slot.npages is an "unsigned long".  The
> discrepancy between a u64 for the offset and a size_t for the size is confusing,
> as they are both conceptually in the same "domain".
>
> Rather than u64 and size_t, we should use pgoff_t, which is what KVM already uses
> as the storage for kvm_memory_slot.gmem.pgoff.
>

I picked size_t more because I thought it was semantically correct to
use the size type for a size. size_t may have different sizes (64 vs
32), but in the comparison offset + size > i_size_read(inode), size is
promoted to 64 bits, and signed inode size is cast to unsigned for
comparison, so I think that works.

pgoff_t is also unsigned, but I think that should be reserved for page
offsets/indices? Is that the way to think when choosing types? What does
"domain" mean above?

> I'll send a new version as a standalone patch.