linux-kernel - Re: [PATCH 1/6] KVM: guest_memfd: Add DEFAULT_SHARED flag, reject user page faults if not set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGtprH-CUMpGqN_68Q_+voJzMpsWnfKKBPmgBGgMgoeTt0E-aw@mail.gmail.com>
Date: Fri, 3 Oct 2025 13:30:02 -0700
From: Vishal Annapurve <vannapurve@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Ackerley Tng <ackerleytng@...gle.com>, David Hildenbrand <david@...hat.com>, 
	Patrick Roy <patrick.roy@...ux.dev>, Fuad Tabba <tabba@...gle.com>, 
	Paolo Bonzini <pbonzini@...hat.com>, Christian Borntraeger <borntraeger@...ux.ibm.com>, 
	Janosch Frank <frankja@...ux.ibm.com>, Claudio Imbrenda <imbrenda@...ux.ibm.com>, 
	kvm list <kvm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Nikita Kalyazin <kalyazin@...zon.co.uk>, Shivank Garg <shivankg@....com>
Subject: Re: [PATCH 1/6] KVM: guest_memfd: Add DEFAULT_SHARED flag, reject
 user page faults if not set

On Fri, Oct 3, 2025 at 9:13 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Thu, Oct 02, 2025, Vishal Annapurve wrote:
> > On Thu, Oct 2, 2025, 5:12 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > >
> > > > >
> > > > > If the _only_ user-visible asset that is added is a KVM_CREATE_GUEST_MEMFD flag,
> > > > > a CAP is gross overkill.  Even if there are other assets that accompany the new
> > > > > flag, there's no reason we couldn't say "this feature exist if XYZ flag is
> > > > > supported".
> > > > >
> > > > > E.g. it's functionally no different than KVM_CAP_VM_TYPES reporting support for
> > > > > KVM_X86_TDX_VM also effectively reporting support for a _huge_ number of things
> > > > > far beyond being able to create a VM of type KVM_X86_TDX_VM.
> > > > >
> > > >
> > > > What's your opinion about having KVM_CAP_GUEST_MEMFD_MMAP part of
> > > > KVM_CAP_GUEST_MEMFD_CAPS i.e. having a KVM cap covering all features
> > > > of guest_memfd?
> > >
> > > I'd much prefer to have both.  Describing flags for an ioctl via a bitmask that
> > > doesn't *exactly* match the flags is asking for problems.  At best, it will be
> > > confusing.  E.g. we'll probably end up with code like this:
> > >
> > >         gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);
> > >
> > >         if (gmem_caps & KVM_CAP_GUEST_MEMFD_MMAP)
> > >                 gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
> > >         if (gmem_caps & KVM_CAP_GUEST_MEMFD_INIT_SHARED)
> > >                 gmem_flags |= KVM_CAP_GUEST_MEMFD_INIT_SHARED;
> > >
> >
> > No, I actually meant the userspace can just rely on the cap to assume
> > right flags to be available (not necessarily the same flags as cap
> > bits).
> >
> > i.e. Userspace will do something like:
> > gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);
> >
> > if (gmem_caps & KVM_CAP_GUEST_MEMFD_MMAP)
> >         gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
> > if (gmem_caps & KVM_CAP_GUEST_MEMFD_HUGETLB)
> >         gmem_flags |= GUEST_MEMFD_FLAG_HUGETLB | GUEST_MEMFD_FLAG_HUGETLB_2MB;
>
> Yes, that's exactly what I said.  But I goofed when copy+pasted and failed to
> do s/KVM_CAP_GUEST_MEMFD_INIT_SHARED/GUEST_MEMFD_FLAG_INIT_SHARED, which is the
> type of bug that ideally just can't happen.
>
> Side topic, I'm not at all convinced that this is what we want for KVM's uAPI:
>
>         if (gmem_caps & KVM_CAP_GUEST_MEMFD_HUGETLB)
>                 gmem_flags |= GUEST_MEMFD_FLAG_HUGETLB | GUEST_MEMFD_FLAG_HUGETLB_2MB;
>
> See https://lore.kernel.org/all/aN_fJEZXo6wkcHOh@google.com.

Ack, that makes sense to me.

>
> > Userspace has to anyways assume flag values, userspace just needs to
> > know if a particular feature is available.
>
> I don't understand what you mean by "assume flag values".

Ok, I think you covered the explanation of why you would prefer to
have KVM_CAP_GUEST_MEMFD_FLAGS around and I misinterpreted some of it.

One more example with KVM_CAP_GUEST_MEMFD_FLAGS around:

gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);
valid_flags = kvm_check_cap(KVM_CAP_GUEST_MEMFD_FLAGS);

if (gmem_caps & KVM_CAP_GUEST_MEMFD_CONVERSION) {
               // Use single memory backing paths for 4K backing
              if (valid_flags & GUEST_MEMFD_FLAG_MMAP)
                          gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
              else
                        // error out;
}
if (gmem_caps & KVM_CAP_GUEST_MEMFD_HUGETLB_CONVERSION) {
               // Use single memory backing paths for hugetlb memory backing
               if (valid_flags & GUEST_MEMFD_FLAG_HUGETLB) {
                          gmem_flags |= GUEST_MEMFD_FLAG_HUGETLB;
                          kvm_create_guest_memfd.huge_page_size_log2 = ...;
               } else
                        // error out;
}

Userspace will have to rely on a combination of flags and caps to
decide it's control flow instead of just caps. Thinking more about
this, I don't have a strong preference between two scenarios i.e. with
or without KVM_CAP_GUEST_MEMFD_FLAGS.