Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 14 Apr 2020 13:32:03 +0000 (UTC)
From:      Kyle Evans <kevans@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r359918 - in head: sys/kern tests/sys/kern
Message-ID:  <202004141332.03EDW3tQ075638@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kevans
Date: Tue Apr 14 13:32:03 2020
New Revision: 359918
URL: https://svnweb.freebsd.org/changeset/base/359918

Log:
  posixshm: fix counting of writable mappings
  
  Similar to mmap'ing vnodes, posixshm should count any mapping where maxprot
  contains VM_PROT_WRITE (i.e. fd opened r/w with no write-seal applied) as
  writable and thus blocking of any write-seal.
  
  The memfd tests have been amended to reflect the fixes here, which notably
  includes:
  
  1. Fix for error return bug; EPERM is not a documented failure mode for mmap
  2. Fix rejection of write-seal with active mappings that can be upgraded via
      mprotect(2).
  
  Reported by:	markj
  Discussed with:	markj, kib

Modified:
  head/sys/kern/uipc_shm.c
  head/tests/sys/kern/memfd_test.c

Modified: head/sys/kern/uipc_shm.c
==============================================================================
--- head/sys/kern/uipc_shm.c	Tue Apr 14 13:12:22 2020	(r359917)
+++ head/sys/kern/uipc_shm.c	Tue Apr 14 13:32:03 2020	(r359918)
@@ -1147,14 +1147,16 @@ shm_mmap(struct file *fp, vm_map_t map, vm_offset_t *a
 		if ((fp->f_flag & FWRITE) != 0 &&
 		    (shmfd->shm_seals & F_SEAL_WRITE) == 0)
 			maxprot |= VM_PROT_WRITE;
-		writecnt = (prot & VM_PROT_WRITE) != 0;
-		if (writecnt && (shmfd->shm_seals & F_SEAL_WRITE) != 0) {
-			error = EPERM;
-			goto out;
-		}
 
-		/* Don't permit shared writable mappings on read-only descriptors. */
-		if (writecnt && (maxprot & VM_PROT_WRITE) == 0) {
+		/*
+		 * Any mappings from a writable descriptor may be upgraded to
+		 * VM_PROT_WRITE with mprotect(2), unless a write-seal was
+		 * applied between the open and subsequent mmap(2).  We want to
+		 * reject application of a write seal as long as any such
+		 * mapping exists so that the seal cannot be trivially bypassed.
+		 */
+		writecnt = (maxprot & VM_PROT_WRITE) != 0;
+		if (!writecnt && (prot & VM_PROT_WRITE) != 0) {
 			error = EACCES;
 			goto out;
 		}

Modified: head/tests/sys/kern/memfd_test.c
==============================================================================
--- head/tests/sys/kern/memfd_test.c	Tue Apr 14 13:12:22 2020	(r359917)
+++ head/tests/sys/kern/memfd_test.c	Tue Apr 14 13:32:03 2020	(r359918)
@@ -108,7 +108,7 @@ ATF_TC_BODY(write_seal, tc)
 
 	ATF_REQUIRE(mmap(0, BUF_SIZE, (PROT_READ | PROT_WRITE), MAP_SHARED,
 	    fd, 0) == MAP_FAILED);
-	ATF_REQUIRE(errno == EPERM);
+	ATF_REQUIRE(errno == EACCES);
 
 	close(fd);
 }
@@ -136,13 +136,23 @@ ATF_TC_BODY(mmap_write_seal, tc)
 
 	ATF_REQUIRE(munmap(addr, BUF_SIZE) == 0);
 
+	/*
+	 * This should fail, because raddr still exists and it was spawned from
+	 * a r/w fd.
+	 */
+	ATF_REQUIRE(fcntl(fd, F_ADD_SEALS, F_SEAL_WRITE) == -1);
+	ATF_REQUIRE(errno == EBUSY);
+
+	ATF_REQUIRE(munmap(raddr, BUF_SIZE) == 0);
+	/* This one should succeed; only the private mapping remains. */
 	ATF_REQUIRE(fcntl(fd, F_ADD_SEALS, F_SEAL_WRITE) == 0);
 
 	ATF_REQUIRE(munmap(paddr, BUF_SIZE) == 0);
-	ATF_REQUIRE(munmap(raddr, BUF_SIZE) == 0);
 	ATF_REQUIRE(mmap(0, BUF_SIZE, (PROT_READ | PROT_WRITE), MAP_SHARED,
 	    fd, 0) == MAP_FAILED);
-	ATF_REQUIRE(errno == EPERM);
+	ATF_REQUIRE(errno == EACCES);
+
+	/* Make sure we can still map privately r/w or shared r/o. */
 	paddr = mmap(0, BUF_SIZE, (PROT_READ | PROT_WRITE), MAP_PRIVATE, fd, 0);
 	ATF_REQUIRE(paddr != MAP_FAILED);
 	raddr = mmap(0, BUF_SIZE, PROT_READ, MAP_SHARED, fd, 0);
@@ -227,7 +237,7 @@ ATF_TC_BODY(dup_seals, tc)
 
 	ATF_REQUIRE(mmap(0, BUF_SIZE, (PROT_READ | PROT_WRITE), MAP_SHARED,
 	    fdx, 0) == MAP_FAILED);
-	ATF_REQUIRE(errno == EPERM);
+	ATF_REQUIRE(errno == EACCES);
 
 	close(fd);
 	close(fdx);
@@ -259,7 +269,6 @@ ATF_TC_BODY(immutable_seals, tc)
 	    "Added duplicate grow seal after restricting seals");
 	close(fd);
 }
-
 
 ATF_TP_ADD_TCS(tp)
 {

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202004141332.03EDW3tQ075638>