[RFC,01/13] fs/userfaultfd: fix wrong error code on WP & !VM_MAYWRITE
diff mbox series

Message ID 20201129004548.1619714-2-namit@vmware.com
State New, archived
Headers show
Series
  • fs/userfaultfd: support iouring and polling
Related show

Commit Message

Nadav Amit Nov. 29, 2020, 12:45 a.m. UTC
From: Nadav Amit <namit@vmware.com>

It is possible to get an EINVAL error instead of EPERM if the following
test vm_flags have VM_UFFD_WP but do not have VM_MAYWRITE, as "ret" is
overwritten since commit cab350afcbc9 ("userfaultfd: hugetlbfs: allow
registration of ranges containing huge pages").

Fix it.

Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: io-uring@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Fixes: cab350afcbc9 ("userfaultfd: hugetlbfs: allow registration of ranges containing huge pages")
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 fs/userfaultfd.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Mike Kravetz Dec. 1, 2020, 9:22 p.m. UTC | #1
On 11/28/20 4:45 PM, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> It is possible to get an EINVAL error instead of EPERM if the following
> test vm_flags have VM_UFFD_WP but do not have VM_MAYWRITE, as "ret" is
> overwritten since commit cab350afcbc9 ("userfaultfd: hugetlbfs: allow
> registration of ranges containing huge pages").
> 
> Fix it.
> 
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: io-uring@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mm@kvack.org
> Fixes: cab350afcbc9 ("userfaultfd: hugetlbfs: allow registration of ranges containing huge pages")
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  fs/userfaultfd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 000b457ad087..c8ed4320370e 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1364,6 +1364,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>  			if (end & (vma_hpagesize - 1))
>  				goto out_unlock;
>  		}
> +		ret = -EPERM;
>  		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE))
>  			goto out_unlock;
>  

Thanks!  We should return EPERM in that case.

However, the check for VM_UFFD_WP && !VM_MAYWRITE went in after commit
cab350afcbc9.  I think it is more accurate to say that the issue was
introduced with commit 63b2d4174c4a ("Introduce the new uffd-wp APIs
for userspace.").  The convention in userfaultfd_register() is that the
return code is set before testing condition which could cause return.
Therefore, when 63b2d4174c4a added the VM_UFFD_WP && !VM_MAYWRITE check,
it should have also added the 'ret = -EPERM;' statement.

With changes to commit message and Fixes tag,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
Peter Xu Dec. 21, 2020, 7:01 p.m. UTC | #2
On Tue, Dec 01, 2020 at 01:22:32PM -0800, Mike Kravetz wrote:
> On 11/28/20 4:45 PM, Nadav Amit wrote:
> > From: Nadav Amit <namit@vmware.com>
> > 
> > It is possible to get an EINVAL error instead of EPERM if the following
> > test vm_flags have VM_UFFD_WP but do not have VM_MAYWRITE, as "ret" is
> > overwritten since commit cab350afcbc9 ("userfaultfd: hugetlbfs: allow
> > registration of ranges containing huge pages").
> > 
> > Fix it.
> > 
> > Cc: Mike Kravetz <mike.kravetz@oracle.com>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Cc: Andrea Arcangeli <aarcange@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: io-uring@vger.kernel.org
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-mm@kvack.org
> > Fixes: cab350afcbc9 ("userfaultfd: hugetlbfs: allow registration of ranges containing huge pages")
> > Signed-off-by: Nadav Amit <namit@vmware.com>
> > ---
> >  fs/userfaultfd.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 000b457ad087..c8ed4320370e 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -1364,6 +1364,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> >  			if (end & (vma_hpagesize - 1))
> >  				goto out_unlock;
> >  		}
> > +		ret = -EPERM;
> >  		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE))
> >  			goto out_unlock;
> >  
> 
> Thanks!  We should return EPERM in that case.
> 
> However, the check for VM_UFFD_WP && !VM_MAYWRITE went in after commit
> cab350afcbc9.  I think it is more accurate to say that the issue was
> introduced with commit 63b2d4174c4a ("Introduce the new uffd-wp APIs
> for userspace.").  The convention in userfaultfd_register() is that the
> return code is set before testing condition which could cause return.
> Therefore, when 63b2d4174c4a added the VM_UFFD_WP && !VM_MAYWRITE check,
> it should have also added the 'ret = -EPERM;' statement.

Right, if there's a "fixes" then it should be the uffd-wp patch.

Though I really think it won't happen... Firstly because hugetlbfs is not yet
supported for uffd-wp, so the two "if" won't collapse, so no way to trigger it
imho. More importantly we've got one check ahead of it:

		/*
		 * UFFDIO_COPY will fill file holes even without
		 * PROT_WRITE. This check enforces that if this is a
		 * MAP_SHARED, the process has write permission to the backing
		 * file. If VM_MAYWRITE is set it also enforces that on a
		 * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
		 * F_WRITE_SEAL can be taken until the vma is destroyed.
		 */
		ret = -EPERM;
		if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
			goto out_unlock;

AFAICT it will fail there directly when write perm is missing.

My wild guess is that the 1st version of 63b2d4174c4ad1f (2020) came earlier
than 29ec90660d (2018), however not needed anymore after the 2020 patch.  Hence
it's probably overlooked by me when I rebased.

Summary: IMHO no bug to fix, but we can directly drop the latter check?

Thanks,

Patch
diff mbox series

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 000b457ad087..c8ed4320370e 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1364,6 +1364,7 @@  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 			if (end & (vma_hpagesize - 1))
 				goto out_unlock;
 		}
+		ret = -EPERM;
 		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE))
 			goto out_unlock;