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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGtprH9N=974HZiqfdaO9DK9nycDD9NeiPeHC49P-DkgTaWtTw@mail.gmail.com>
Date: Thu, 2 Oct 2025 21:10:47 -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 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;

Userspace has to anyways assume flag values, userspace just needs to
know if a particular feature is available.

> ...
> Another issue is that, while unlikely, we could run out of KVM_CAP_GUEST_MEMFD_CAPS
> bits before we run out of flags.

I would say that's unlikely as I know of at least one feature that
needs multiple flag bits.

>
> And if we use memory attributes, we're also guaranteed to have at least one gmem
> capability that returns a bitmask separately from a dedicated one-size-fits-all
> cap, e.g.
>
>         case KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES:
>                 if (vm_memory_attributes)
>                         return 0;
>
>                 return kvm_supported_mem_attributes(kvm);

For this one, we need a separate dedicated cap.

>
> Side topic, looking at this, I don't think we need KVM_CAP_GUEST_MEMFD_CAPS, I'm
> pretty sure we can simply extend KVM_CAP_GUEST_MEMFD.  E.g.
>
> #define KVM_GUEST_MEMFD_FEAT_BASIC              (1ULL << 0)
> #define KVM_GUEST_MEMFD_FEAT_FANCY              (1ULL << 1)
>
>         case KVM_CAP_GUEST_MEMFD:
>                 return KVM_GUEST_MEMFD_FEAT_BASIC |
>                        KVM_GUEST_MEMFD_FEAT_FANCY;

This scheme seems ok to me.

>
> > That seems more consistent to me in order for userspace to deduce the
> > supported features and assume flags/ioctls/...  associated with the feature
> > as a group.
>
> If we add a feature that comes with a flag, we could always add both, i.e. a
> feature flag for KVM_CAP_GUEST_MEMFD along with the natural enumeration for
> KVM_CAP_GUEST_MEMFD_FLAGS.  That certainly wouldn't be my first choice, but it's
> a possibility, e.g. if it really is the most intuitive solution.  But that's
> getting quite hypothetical.