linux-kernel - Re: [RFC PATCH] KVM: TDX: Decouple TDX init mem region from kvm_gmem

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: <aHFx2wtHcfimVKW_@google.com>
Date: Fri, 11 Jul 2025 13:19:39 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Michael Roth <michael.roth@....com>
Cc: Vishal Annapurve <vannapurve@...gle.com>, Yan Zhao <yan.y.zhao@...el.com>, pbonzini@...hat.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, rick.p.edgecombe@...el.com, 
	kai.huang@...el.com, adrian.hunter@...el.com, reinette.chatre@...el.com, 
	xiaoyao.li@...el.com, tony.lindgren@...el.com, binbin.wu@...ux.intel.com, 
	dmatlack@...gle.com, isaku.yamahata@...el.com, ira.weiny@...el.com, 
	david@...hat.com, ackerleytng@...gle.com, tabba@...gle.com, 
	chao.p.peng@...el.com
Subject: Re: [RFC PATCH] KVM: TDX: Decouple TDX init mem region from kvm_gmem_populate()

On Fri, Jul 11, 2025, Michael Roth wrote:
> On Fri, Jul 11, 2025 at 11:38:10AM -0700, Vishal Annapurve wrote:
> > On Fri, Jul 11, 2025 at 9:37 AM Michael Roth <michael.roth@....com> wrote:
> > >
> > > >
> > > > static long __kvm_gmem_populate(struct kvm *kvm, struct kvm_memory_slot *slot,
> > > >                               struct file *file, gfn_t gfn, void __user *src,
> > > >                               kvm_gmem_populate_cb post_populate, void *opaque)
> > > > {
> > > >       pgoff_t index = kvm_gmem_get_index(slot, gfn);
> > > >       struct page *src_page = NULL;
> > > >       bool is_prepared = false;
> > > >       struct folio *folio;
> > > >       int ret, max_order;
> > > >       kvm_pfn_t pfn;
> > > >
> > > >       if (src) {
> > > >               ret = get_user_pages((unsigned long)src, 1, 0, &src_page);
> > > >               if (ret < 0)
> > > >                       return ret;
> > > >               if (ret != 1)
> > > >                       return -ENOMEM;
> > > >       }
> > >
> > > One tricky part here is that the uAPI currently expects the pages to
> > > have the private attribute set prior to calling kvm_gmem_populate(),
> > > which gets enforced below.
> > >
> > > For in-place conversion: the idea is that userspace will convert
> > > private->shared to update in-place, then immediately convert back
> > > shared->private; so that approach would remain compatible with above
> > > behavior. But if we pass a 'src' parameter to kvm_gmem_populate(),
> > > and do a GUP or copy_from_user() on it at any point, regardless if
> > > it is is outside of filemap_invalidate_lock(), then
> > > kvm_gmem_fault_shared() will return -EACCES.
> > 
> > I think that's a fine way to fail the initial memory population, this
> > simply means userspace didn't pass the right source address. Why do we
> > have to work around this error? Userspace should simply pass the
> > source buffer that is accessible to the host or pass null to indicate
> > that the target gfn already has the needed contents.
> > 
> > That is, userspace can still bring a separate source buffer even with
> > in-place conversion available.

Yeah.  It might be superfluous to a certain extent, and it should be straight up
disallowed with PRESERVE, but I don't like the idea of taking a hard dependency
on src being NULL.

> I thought there was some agreement that mmap() be the 'blessed'
> approach for initializing memory with in-place conversion to help
> untangle some of these paths, so it made sense to enforce that in
> kvm_gmem_populate() to make it 'official', but with Sean's suggested
> rework I suppose we could support both approaches.

Ya, my preference would be to not rely on subtly making two paths mutually
exclusive in order to avoid deadlock, especially when there are ABI implications.

I'm not dead set against it, e.g. if for some reason we just absolutely need to
disallow a non-NULL src for the that case, but hopefully we can avoid that.