| 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
| ||
|
Message-ID: <CADrL8HV8+dh4xPv6Da5CR+CwGJwg5uHyNmiVmHhWFJSwy8ChRw@mail.gmail.com>
Date: Wed, 10 Sep 2025 14:23:18 -0700
From: James Houghton <jthoughton@...gle.com>
To: "Kalyazin, Nikita" <kalyazin@...zon.co.uk>
Cc: "pbonzini@...hat.com" <pbonzini@...hat.com>, "shuah@...nel.org" <shuah@...nel.org>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "michael.day@....com" <michael.day@....com>,
"david@...hat.com" <david@...hat.com>, "Roy, Patrick" <roypat@...zon.co.uk>,
"Thomson, Jack" <jackabt@...zon.co.uk>, "Manwaring, Derek" <derekmn@...zon.com>,
"Cali, Marco" <xmarcalx@...zon.co.uk>
Subject: Re: [PATCH v5 1/2] KVM: guest_memfd: add generic population via write
On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita <kalyazin@...zon.co.uk> wrote:
>
> From: Nikita Kalyazin <kalyazin@...zon.com>
Hi Nikita,
>
> write syscall populates guest_memfd with user-supplied data in a generic
> way, ie no vendor-specific preparation is performed. This is supposed
> to be used in non-CoCo setups where guest memory is not
> hardware-encrypted.
What's meant to happen if we do use this for CoCo VMs? I would expect
write() to fail, but I don't see why it would (seems like we need/want
a check that we aren't write()ing to private memory).
> The following behaviour is implemented:
> - only page-aligned count and offset are allowed
> - if the memory is already allocated, the call will successfully
> populate it
> - if the memory is not allocated, the call will both allocate and
> populate
> - if the memory is already populated, the call will not repopulate it
>
> Signed-off-by: Nikita Kalyazin <kalyazin@...zon.com>
> ---
> virt/kvm/guest_memfd.c | 64 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 08a6bc7d25b6..a2e86ec13e4b 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -379,7 +379,9 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
> }
>
> static struct file_operations kvm_gmem_fops = {
> - .mmap = kvm_gmem_mmap,
> + .mmap = kvm_gmem_mmap,
> + .llseek = default_llseek,
> + .write_iter = generic_perform_write,
You seem to have accidentally replaced some tabs with spaces here. :)
Please keep the style consistent.
> .open = generic_file_open,
> .release = kvm_gmem_release,
> .fallocate = kvm_gmem_fallocate,
> @@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module)
> kvm_gmem_fops.owner = module;
> }
>
> +static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,
> + struct address_space *mapping,
> + loff_t pos, unsigned int len,
> + struct folio **foliop,
> + void **fsdata)
> +{
> + struct file *file = kiocb->ki_filp;
> + pgoff_t index = pos >> PAGE_SHIFT;
> + struct folio *folio;
> +
> + if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
> + return -EINVAL;
Requiring pos to be page-aligned seems like a strange restriction, and
requiring len to be exactly PAGE_SIZE just seems wrong. I don't see
any reason why the below logic can't be made to work with an
unrestricted pos and len (in other words, I don't see how guest_memfd
is special vs other filesystems in this regard).
> +
> + if (pos + len > i_size_read(file_inode(file)))
> + return -EINVAL;
> +
> + folio = kvm_gmem_get_folio(file_inode(file), index);
> + if (IS_ERR(folio))
> + return -EFAULT;
> +
> + if (WARN_ON_ONCE(folio_test_large(folio))) {
> + folio_unlock(folio);
> + folio_put(folio);
> + return -EFAULT;
> + }
> +
> + if (folio_test_uptodate(folio)) {
> + folio_unlock(folio);
> + folio_put(folio);
> + return -ENOSPC;
Does it actually matter for the folio not to be uptodate? It seems
unnecessarily restrictive not to be able to overwrite data if we're
saying that this is only usable for unencrypted memory anyway.
Is ENOSPC really the right errno for this? (Maybe -EFAULT?)
> + }
> +
> + *foliop = folio;
> + return 0;
> +}
> +
> +static int kvm_kmem_gmem_write_end(const struct kiocb *kiocb,
> + struct address_space *mapping,
> + loff_t pos, unsigned int len,
> + unsigned int copied,
> + struct folio *folio, void *fsdata)
> +{
> + if (copied) {
> + if (copied < len) {
> + unsigned int from = pos & (PAGE_SIZE - 1);
How about:
unsigned int from = pos & ((1UL << folio_order(*folio)) - 1)
So that we don't need to require !folio_test_large() in
kvm_kmem_gmem_write_begin().
> +
> + folio_zero_range(folio, from + copied, len - copied);
> + }
> + kvm_gmem_mark_prepared(folio);
> + }
> +
> + folio_unlock(folio);
> + folio_put(folio);
> +
> + return copied;
> +}
> +
> static int kvm_gmem_migrate_folio(struct address_space *mapping,
> struct folio *dst, struct folio *src,
> enum migrate_mode mode)
> @@ -442,6 +501,8 @@ static void kvm_gmem_free_folio(struct folio *folio)
>
> static const struct address_space_operations kvm_gmem_aops = {
> .dirty_folio = noop_dirty_folio,
> + .write_begin = kvm_kmem_gmem_write_begin,
> + .write_end = kvm_kmem_gmem_write_end,
> .migrate_folio = kvm_gmem_migrate_folio,
> .error_remove_folio = kvm_gmem_error_folio,
> #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
> @@ -489,6 +550,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> }
>
> file->f_flags |= O_LARGEFILE;
> + file->f_mode |= FMODE_LSEEK | FMODE_PWRITE;
>
> inode = file->f_inode;
> WARN_ON(file->f_mapping != inode->i_mapping);
> --
> 2.50.1
>
Powered by blists - more mailing lists