Re: [PATCH v9 08/17] KVM: guest_memfd: Check that userspace_addr and fd+offset refer to same range

[Sean Christopherson <seanjc@google.com>]

On Wed, May 14, 2025, Ackerley Tng wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > On Wed, May 14, 2025, Fuad Tabba wrote:
> >> On Tue, 13 May 2025 at 21:31, James Houghton <jthoughton@google.com> wrote:
> >> > > @@ -585,9 +611,14 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> >> > >             offset + size > i_size_read(inode))
> >> > >                 goto err;
> >> > >
> >> > > -       if (kvm_gmem_supports_shared(inode) &&
> >> > > -           !kvm_arch_vm_supports_gmem_shared_mem(kvm))
> >> > > -               goto err;
> >> > > +       if (kvm_gmem_supports_shared(inode)) {
> >> > > +               if (!kvm_arch_vm_supports_gmem_shared_mem(kvm))
> >> > > +                       goto err;
> >> > > +
> >> > > +               if (slot->userspace_addr &&
> >> > > +                   !kvm_gmem_is_same_range(kvm, slot, file, offset))
> >> > > +                       goto err;
> >> >
> >> > This is very nit-picky, but I would rather this not be -EINVAL, maybe
> >> > -EIO instead? Or maybe a pr_warn_once() and let the call proceed?
> >
> > Or just omit the check entirely.  The check isn't binding (ba-dump, ching!),
> > because the mapping/VMA can change the instant mmap_read_unlock() is called.
> >
> >> > The userspace_addr we got isn't invalid per se, we're just trying to
> >> > give a hint to the user that their VMAs (or the userspace address they
> >> > gave us) are messed up. I don't really like lumping this in with truly
> >> > invalid arguments.
> >> 
> >> I don't mind changing the return error, but I don't think that we
> >> should have a kernel warning (pr_warn_once) for something userspace
> >> can trigger.
> >
> > This isn't a WARN, e.g. won't trip panic_on_warn.  In practice, it's not
> > meaningfully different than pr_info().  That said, I agree that printing anything
> > is a bad approach.
> >
> >> It's not an IO error either. I think that this is an invalid argument
> >> (EINVAL).
> >
> > I agree with James, this isn't an invalid argument.  Having the validity of an
> > input hinge on the ordering between a KVM ioctl() and mmap() is quite odd.  I
> > know KVM arm64 does exactly this for KVM_SET_USER_MEMORY_REGION{,2}, but I don't
> > love the semantics.  And unlike that scenario, where e.g. MTE tags are verified
> > again at fault-time, KVM won't re-check the VMA when accessing guest memory via
> > the userspace mapping, e.g. through uaccess.
> >
> > Unless I'm forgetting something, I'm leaning toward omitting the check entirely.
> >
> 
> I'm good with dropping this patch. I might have misunderstood the conclusion
> of the guest_memfd call.

No, I don't think you misunderstood anything.  It's just that sometimes opinions
different when there's actual code, versus a verbal discussion.  I.e. this sounds
like a good idea, but when seeing the code and thinking through the effects, it's
less appealing.