linux-kernel - Re: [PATCH v3 2/3] userfaultfd: UFFDIO

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]

Date:   Mon, 23 Oct 2023 14:29:34 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Suren Baghdasaryan <surenb@...gle.com>, akpm@...ux-foundation.org
Cc:     viro@...iv.linux.org.uk, brauner@...nel.org, shuah@...nel.org,
        aarcange@...hat.com, lokeshgidra@...gle.com, peterx@...hat.com,
        hughd@...gle.com, mhocko@...e.com, axelrasmussen@...gle.com,
        rppt@...nel.org, willy@...radead.org, Liam.Howlett@...cle.com,
        jannh@...gle.com, zhangpeng362@...wei.com, bgeffon@...gle.com,
        kaleshsingh@...gle.com, ngeoffray@...gle.com, jdduke@...gle.com,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        kernel-team@...roid.com
Subject: Re: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI

Focusing on validate_remap_areas():

> +
> +static int validate_remap_areas(struct vm_area_struct *src_vma,
> +				struct vm_area_struct *dst_vma)
> +{
> +	/* Only allow remapping if both have the same access and protection */
> +	if ((src_vma->vm_flags & VM_ACCESS_FLAGS) != (dst_vma->vm_flags & VM_ACCESS_FLAGS) ||
> +	    pgprot_val(src_vma->vm_page_prot) != pgprot_val(dst_vma->vm_page_prot))
> +		return -EINVAL;

Makes sense. I do wonder about pkey and friends and if we even have to 
so anything special.

> +
> +	/* Only allow remapping if both are mlocked or both aren't */
> +	if ((src_vma->vm_flags & VM_LOCKED) != (dst_vma->vm_flags & VM_LOCKED))
> +		return -EINVAL;
> +
> +	if (!(src_vma->vm_flags & VM_WRITE) || !(dst_vma->vm_flags & VM_WRITE))
> +		return -EINVAL;

Why does one of both need VM_WRITE? If one really needs it, then the 
destination (where we're moving stuff to).

> +
> +	/*
> +	 * Be strict and only allow remap_pages if either the src or
> +	 * dst range is registered in the userfaultfd to prevent
> +	 * userland errors going unnoticed. As far as the VM
> +	 * consistency is concerned, it would be perfectly safe to
> +	 * remove this check, but there's no useful usage for
> +	 * remap_pages ouside of userfaultfd registered ranges. This
> +	 * is after all why it is an ioctl belonging to the
> +	 * userfaultfd and not a syscall.

I think the last sentence is the important bit and the comment can 
likely be reduced.

> +	 *
> +	 * Allow both vmas to be registered in the userfaultfd, just
> +	 * in case somebody finds a way to make such a case useful.
> +	 * Normally only one of the two vmas would be registered in
> +	 * the userfaultfd.

Should we just check the destination? That makes most sense to me, 
because with uffd we are resolving uffd-events. And just like 
copy/zeropage we want to resolve a page fault ("userfault") of a 
non-present page on the destination.


> +	 */
> +	if (!dst_vma->vm_userfaultfd_ctx.ctx &&
> +	    !src_vma->vm_userfaultfd_ctx.ctx)
> +		return -EINVAL;



> +
> +	/*
> +	 * FIXME: only allow remapping across anonymous vmas,
> +	 * tmpfs should be added.
> +	 */
> +	if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma))
> +		return -EINVAL;

Why a FIXME here? Just drop the comment completely or replace it with 
"We only allow to remap anonymous folios accross anonymous VMAs".

> +
> +	/*
> +	 * Ensure the dst_vma has a anon_vma or this page
> +	 * would get a NULL anon_vma when moved in the
> +	 * dst_vma.
> +	 */
> +	if (unlikely(anon_vma_prepare(dst_vma)))
> +		return -ENOMEM;

Makes sense.

> +
> +	return 0;
> +}


-- 
Cheers,

David / dhildenb