linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
@ 2019-11-07 19:53 Joel Fernandes (Google)
  2019-11-07 19:53 ` [PATCH 2/2] memfd: Add test for COW " Joel Fernandes (Google)
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Joel Fernandes (Google) @ 2019-11-07 19:53 UTC (permalink / raw)
  To: linux-kernel
  Cc: Nicolas Geoffray, kernel-team, Joel Fernandes, Andrew Morton,
	Hugh Dickins, linux-kselftest, linux-mm, Shuah Khan

From: Nicolas Geoffray <ngeoffray@google.com>

F_SEAL_FUTURE_WRITE has unexpected behavior when used with MAP_PRIVATE:
A private mapping created after the memfd file that gets sealed with
F_SEAL_FUTURE_WRITE loses the copy-on-write at fork behavior, meaning
children and parent share the same memory, even though the mapping is
private.

The reason for this is due to the code below:

static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
{
        struct shmem_inode_info *info = SHMEM_I(file_inode(file));

        if (info->seals & F_SEAL_FUTURE_WRITE) {
                /*
                 * New PROT_WRITE and MAP_SHARED mmaps are not allowed when
                 * "future write" seal active.
                 */
                if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_WRITE))
                        return -EPERM;

                /*
                 * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED
                 * read-only mapping, take care to not allow mprotect to revert
                 * protections.
                 */
                vma->vm_flags &= ~(VM_MAYWRITE);
        }
        ...
}

And for the mm to know if a mapping is copy-on-write:
static inline bool is_cow_mapping(vm_flags_t flags)
{
        return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
}

The patch fixes the issue by making the mprotect revert protection
happen only for shared mappings. For private mappings, using mprotect
will have no effect on the seal behavior.

Cc: kernel-team@android.com
Signed-off-by: Nicolas Geoffray <ngeoffray@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

---
Google bug: 143833776

 mm/shmem.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 447fd575587c..6ac5e867ef13 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2214,11 +2214,14 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
 			return -EPERM;
 
 		/*
-		 * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED
-		 * read-only mapping, take care to not allow mprotect to revert
-		 * protections.
+		 * Since an F_SEAL_FUTURE_WRITE sealed memfd can be mapped as
+		 * MAP_SHARED and read-only, take care to not allow mprotect to
+		 * revert protections on such mappings. Do this only for shared
+		 * mappings. For private mappings, don't need to mask VM_MAYWRITE
+		 * as we still want them to be COW-writable.
 		 */
-		vma->vm_flags &= ~(VM_MAYWRITE);
+		if (vma->vm_flags & VM_SHARED)
+			vma->vm_flags &= ~(VM_MAYWRITE);
 	}
 
 	file_accessed(file);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] memfd: Add test for COW on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-07 19:53 [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings Joel Fernandes (Google)
@ 2019-11-07 19:53 ` Joel Fernandes (Google)
  2019-11-08  1:00 ` [PATCH 1/2] memfd: Fix COW issue " Andrew Morton
  2019-11-08  6:33 ` Christoph Hellwig
  2 siblings, 0 replies; 9+ messages in thread
From: Joel Fernandes (Google) @ 2019-11-07 19:53 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Andrew Morton, Hugh Dickins, kernel-team, linux-kselftest,
	linux-mm, Shuah Khan

In this test, the parent and child both have writable private mappings.
The test shows that without the patch in this series, the parent and
child shared the same memory which is incorrect. In other words, COW
needs to be triggered so any writes to child's copy stays local to the
child.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 tools/testing/selftests/memfd/memfd_test.c | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index c67d32eeb668..334a7eea2004 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -290,6 +290,40 @@ static void mfd_assert_read_shared(int fd)
 	munmap(p, mfd_def_size);
 }
 
+static void mfd_assert_fork_private_write(int fd)
+{
+	int *p;
+	pid_t pid;
+
+	p = mmap(NULL,
+		 mfd_def_size,
+		 PROT_READ | PROT_WRITE,
+		 MAP_PRIVATE,
+		 fd,
+		 0);
+	if (p == MAP_FAILED) {
+		printf("mmap() failed: %m\n");
+		abort();
+	}
+
+	p[0] = 22;
+
+	pid = fork();
+	if (pid == 0) {
+		p[0] = 33;
+		exit(0);
+	} else {
+		waitpid(pid, NULL, 0);
+
+		if (p[0] != 22) {
+			printf("MAP_PRIVATE copy-on-write failed: %m\n");
+			abort();
+		}
+	}
+
+	munmap(p, mfd_def_size);
+}
+
 static void mfd_assert_write(int fd)
 {
 	ssize_t l;
@@ -760,6 +794,8 @@ static void test_seal_future_write(void)
 	mfd_assert_read_shared(fd2);
 	mfd_fail_write(fd2);
 
+	mfd_assert_fork_private_write(fd);
+
 	munmap(p, mfd_def_size);
 	close(fd2);
 	close(fd);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-07 19:53 [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings Joel Fernandes (Google)
  2019-11-07 19:53 ` [PATCH 2/2] memfd: Add test for COW " Joel Fernandes (Google)
@ 2019-11-08  1:00 ` Andrew Morton
  2019-11-08  2:06   ` Joel Fernandes
  2019-11-08  6:33 ` Christoph Hellwig
  2 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2019-11-08  1:00 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Nicolas Geoffray, kernel-team, Hugh Dickins,
	linux-kselftest, linux-mm, Shuah Khan

On Thu,  7 Nov 2019 14:53:54 -0500 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:

> F_SEAL_FUTURE_WRITE has unexpected behavior when used with MAP_PRIVATE:
> A private mapping created after the memfd file that gets sealed with
> F_SEAL_FUTURE_WRITE loses the copy-on-write at fork behavior, meaning
> children and parent share the same memory, even though the mapping is
> private.

That sounds fairly serious.  Should this be backported into -stable kernels?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-08  1:00 ` [PATCH 1/2] memfd: Fix COW issue " Andrew Morton
@ 2019-11-08  2:06   ` Joel Fernandes
  2019-11-08  3:25     ` Andrew Morton
  2019-11-08  6:37     ` Greg KH
  0 siblings, 2 replies; 9+ messages in thread
From: Joel Fernandes @ 2019-11-08  2:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, Nicolas Geoffray, kernel-team, Hugh Dickins,
	linux-kselftest, linux-mm, Shuah Khan

On Thu, Nov 07, 2019 at 05:00:23PM -0800, Andrew Morton wrote:
> On Thu,  7 Nov 2019 14:53:54 -0500 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> 
> > F_SEAL_FUTURE_WRITE has unexpected behavior when used with MAP_PRIVATE:
> > A private mapping created after the memfd file that gets sealed with
> > F_SEAL_FUTURE_WRITE loses the copy-on-write at fork behavior, meaning
> > children and parent share the same memory, even though the mapping is
> > private.
> 
> That sounds fairly serious.  Should this be backported into -stable kernels?

Yes, it should be. The F_SEAL_FUTURE_WRITE feature was introduced in v5.1 so
v5.3.x stable kernels would need a backport. I can submit a backport tomorrow
unless we are Ok with stable automatically picking it up (I believe the
stable folks "auto select" fixes which should detect this is a fix since I
have said it is a fix in the subject line).

thanks,

 - Joel


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-08  2:06   ` Joel Fernandes
@ 2019-11-08  3:25     ` Andrew Morton
  2019-11-08  6:37     ` Greg KH
  1 sibling, 0 replies; 9+ messages in thread
From: Andrew Morton @ 2019-11-08  3:25 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: linux-kernel, Nicolas Geoffray, kernel-team, Hugh Dickins,
	linux-kselftest, linux-mm, Shuah Khan

On Thu, 7 Nov 2019 21:06:14 -0500 Joel Fernandes <joel@joelfernandes.org> wrote:

> On Thu, Nov 07, 2019 at 05:00:23PM -0800, Andrew Morton wrote:
> > On Thu,  7 Nov 2019 14:53:54 -0500 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> > 
> > > F_SEAL_FUTURE_WRITE has unexpected behavior when used with MAP_PRIVATE:
> > > A private mapping created after the memfd file that gets sealed with
> > > F_SEAL_FUTURE_WRITE loses the copy-on-write at fork behavior, meaning
> > > children and parent share the same memory, even though the mapping is
> > > private.
> > 
> > That sounds fairly serious.  Should this be backported into -stable kernels?
> 
> Yes, it should be.

I added

Fixes: ab3948f58ff84 ("mm/memfd: add an F_SEAL_FUTURE_WRITE seal to memfd")
Cc: <stable@vger.kernel.org>

> The F_SEAL_FUTURE_WRITE feature was introduced in v5.1 so
> v5.3.x stable kernels would need a backport. I can submit a backport tomorrow
> unless we are Ok with stable automatically picking it up (I believe the
> stable folks "auto select" fixes which should detect this is a fix since I
> have said it is a fix in the subject line).

The Cc:stable tag should trigger the appropriate actions, assisted by
the Fixes:.  I doubt if "fix" in the Subject has much effect.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-07 19:53 [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings Joel Fernandes (Google)
  2019-11-07 19:53 ` [PATCH 2/2] memfd: Add test for COW " Joel Fernandes (Google)
  2019-11-08  1:00 ` [PATCH 1/2] memfd: Fix COW issue " Andrew Morton
@ 2019-11-08  6:33 ` Christoph Hellwig
  2019-11-08 15:35   ` Joel Fernandes
  2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2019-11-08  6:33 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Nicolas Geoffray, kernel-team, Andrew Morton,
	Hugh Dickins, linux-kselftest, linux-mm, Shuah Khan

> -		 * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED
> -		 * read-only mapping, take care to not allow mprotect to revert
> -		 * protections.
> +		 * Since an F_SEAL_FUTURE_WRITE sealed memfd can be mapped as
> +		 * MAP_SHARED and read-only, take care to not allow mprotect to
> +		 * revert protections on such mappings. Do this only for shared
> +		 * mappings. For private mappings, don't need to mask VM_MAYWRITE

This adds an > 80 char line.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-08  2:06   ` Joel Fernandes
  2019-11-08  3:25     ` Andrew Morton
@ 2019-11-08  6:37     ` Greg KH
  2019-11-08 15:34       ` Joel Fernandes
  1 sibling, 1 reply; 9+ messages in thread
From: Greg KH @ 2019-11-08  6:37 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Andrew Morton, linux-kernel, Nicolas Geoffray, kernel-team,
	Hugh Dickins, linux-kselftest, linux-mm, Shuah Khan

On Thu, Nov 07, 2019 at 09:06:14PM -0500, Joel Fernandes wrote:
> On Thu, Nov 07, 2019 at 05:00:23PM -0800, Andrew Morton wrote:
> > On Thu,  7 Nov 2019 14:53:54 -0500 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> > 
> > > F_SEAL_FUTURE_WRITE has unexpected behavior when used with MAP_PRIVATE:
> > > A private mapping created after the memfd file that gets sealed with
> > > F_SEAL_FUTURE_WRITE loses the copy-on-write at fork behavior, meaning
> > > children and parent share the same memory, even though the mapping is
> > > private.
> > 
> > That sounds fairly serious.  Should this be backported into -stable kernels?
> 
> Yes, it should be. The F_SEAL_FUTURE_WRITE feature was introduced in v5.1 so
> v5.3.x stable kernels would need a backport. I can submit a backport tomorrow
> unless we are Ok with stable automatically picking it up (I believe the
> stable folks "auto select" fixes which should detect this is a fix since I
> have said it is a fix in the subject line).

Never rely on "auto select" to pick up a patch for stable if you already
know it should go to stable.  Just mark it as such, or tell stable@vger
after the fact.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-08  6:37     ` Greg KH
@ 2019-11-08 15:34       ` Joel Fernandes
  0 siblings, 0 replies; 9+ messages in thread
From: Joel Fernandes @ 2019-11-08 15:34 UTC (permalink / raw)
  To: Greg KH
  Cc: Andrew Morton, linux-kernel, Nicolas Geoffray, kernel-team,
	Hugh Dickins, linux-kselftest, linux-mm, Shuah Khan

On Fri, Nov 08, 2019 at 07:37:15AM +0100, Greg KH wrote:
> On Thu, Nov 07, 2019 at 09:06:14PM -0500, Joel Fernandes wrote:
> > On Thu, Nov 07, 2019 at 05:00:23PM -0800, Andrew Morton wrote:
> > > On Thu,  7 Nov 2019 14:53:54 -0500 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> > > 
> > > > F_SEAL_FUTURE_WRITE has unexpected behavior when used with MAP_PRIVATE:
> > > > A private mapping created after the memfd file that gets sealed with
> > > > F_SEAL_FUTURE_WRITE loses the copy-on-write at fork behavior, meaning
> > > > children and parent share the same memory, even though the mapping is
> > > > private.
> > > 
> > > That sounds fairly serious.  Should this be backported into -stable kernels?
> > 
> > Yes, it should be. The F_SEAL_FUTURE_WRITE feature was introduced in v5.1 so
> > v5.3.x stable kernels would need a backport. I can submit a backport tomorrow
> > unless we are Ok with stable automatically picking it up (I believe the
> > stable folks "auto select" fixes which should detect this is a fix since I
> > have said it is a fix in the subject line).
> 
> Never rely on "auto select" to pick up a patch for stable if you already
> know it should go to stable.  Just mark it as such, or tell stable@vger
> after the fact.

Sure, agreed.

Thanks Andrew for adding the tags!

thanks,

 - Joel


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings
  2019-11-08  6:33 ` Christoph Hellwig
@ 2019-11-08 15:35   ` Joel Fernandes
  0 siblings, 0 replies; 9+ messages in thread
From: Joel Fernandes @ 2019-11-08 15:35 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, Nicolas Geoffray, kernel-team, Andrew Morton,
	Hugh Dickins, linux-kselftest, linux-mm, Shuah Khan

On Thu, Nov 07, 2019 at 10:33:08PM -0800, Christoph Hellwig wrote:
> > -		 * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED
> > -		 * read-only mapping, take care to not allow mprotect to revert
> > -		 * protections.
> > +		 * Since an F_SEAL_FUTURE_WRITE sealed memfd can be mapped as
> > +		 * MAP_SHARED and read-only, take care to not allow mprotect to
> > +		 * revert protections on such mappings. Do this only for shared
> > +		 * mappings. For private mappings, don't need to mask VM_MAYWRITE
> 
> This adds an > 80 char line.

Oh, true. Sorry. Andrew I hate to ask you but since you took the patch
already, could you just the comment for the character limit in the one
you applied?

thanks,

 - Joel


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-11-08 15:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07 19:53 [PATCH 1/2] memfd: Fix COW issue on MAP_PRIVATE and F_SEAL_FUTURE_WRITE mappings Joel Fernandes (Google)
2019-11-07 19:53 ` [PATCH 2/2] memfd: Add test for COW " Joel Fernandes (Google)
2019-11-08  1:00 ` [PATCH 1/2] memfd: Fix COW issue " Andrew Morton
2019-11-08  2:06   ` Joel Fernandes
2019-11-08  3:25     ` Andrew Morton
2019-11-08  6:37     ` Greg KH
2019-11-08 15:34       ` Joel Fernandes
2019-11-08  6:33 ` Christoph Hellwig
2019-11-08 15:35   ` Joel Fernandes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).