git: 8635c1ad5127 - stable/14 - shm: Respect PROT_MAX when creating private mappings

From: Mark Johnston <markj_at_FreeBSD.org>
Date: Mon, 04 Nov 2024 15:38:41 UTC
The branch stable/14 has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=8635c1ad51274872d01075c7f66aa252491b1de1

commit 8635c1ad51274872d01075c7f66aa252491b1de1
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2024-10-04 14:54:44 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2024-11-04 15:38:35 +0000

    shm: Respect PROT_MAX when creating private mappings
    
    We were previously unconditionally adding PROT_WRITE to the maxprot of
    private mapping (because a private mapping can be written even if the
    fd is read-only), but this might violate the user's PROT_MAX request.
    
    While here, rename cap_maxprot to max_maxprot.  This is the intersection
    of the maximum protections imposed by capsicum rights on the fd (not
    really relevant for private mappings) and the user-required maximum
    protections (which were not being obeyed).  In particular, cap_maxprot
    is a misnomer after the introduction of PROT_MAX.
    
    Add some regression test cases.  mmap__maxprot_shm fails without this
    patch.
    
    Note: Capsicum's CAP_MMAP_W is a bit ambiguous.  Should it be required
    in order to create writeable private mappings?  Currently it is, even
    though such mappings don't permit writes to the object referenced by the
    fd.
    
    Reported by:    brooks
    Reviewed by:    brooks
    MFC after:      1 month
    Fixes:          c7841c6b8e41 ("Relax restrictions on private mappings of POSIX shm objects.")
    Differential Revision:  https://reviews.freebsd.org/D46741
    
    (cherry picked from commit 33c2c58f0a3db0a6d3996fa14ac7967274678771)
---
 sys/kern/uipc_shm.c                |  8 ++---
 tests/sys/posixshm/posixshm_test.c | 29 +++++++++++++++-
 tests/sys/vm/mmap_test.c           | 70 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/sys/kern/uipc_shm.c b/sys/kern/uipc_shm.c
index 8a533428790a..f51998d0ed00 100644
--- a/sys/kern/uipc_shm.c
+++ b/sys/kern/uipc_shm.c
@@ -1648,7 +1648,7 @@ fail:
 
 static int
 shm_mmap(struct file *fp, vm_map_t map, vm_offset_t *addr, vm_size_t objsize,
-    vm_prot_t prot, vm_prot_t cap_maxprot, int flags,
+    vm_prot_t prot, vm_prot_t max_maxprot, int flags,
     vm_ooffset_t foff, struct thread *td)
 {
 	struct shmfd *shmfd;
@@ -1671,8 +1671,8 @@ shm_mmap(struct file *fp, vm_map_t map, vm_offset_t *addr, vm_size_t objsize,
 	 * writeable.
 	 */
 	if ((flags & MAP_SHARED) == 0) {
-		cap_maxprot |= VM_PROT_WRITE;
-		maxprot |= VM_PROT_WRITE;
+		if ((max_maxprot & VM_PROT_WRITE) != 0)
+			maxprot |= VM_PROT_WRITE;
 		writecnt = false;
 	} else {
 		if ((fp->f_flag & FWRITE) != 0 &&
@@ -1692,7 +1692,7 @@ shm_mmap(struct file *fp, vm_map_t map, vm_offset_t *addr, vm_size_t objsize,
 			goto out;
 		}
 	}
-	maxprot &= cap_maxprot;
+	maxprot &= max_maxprot;
 
 	/* See comment in vn_mmap(). */
 	if (
diff --git a/tests/sys/posixshm/posixshm_test.c b/tests/sys/posixshm/posixshm_test.c
index c97a10bb0a86..a3ce18f855f5 100644
--- a/tests/sys/posixshm/posixshm_test.c
+++ b/tests/sys/posixshm/posixshm_test.c
@@ -1191,6 +1191,33 @@ ATF_TC_BODY(accounting, tc)
 	ATF_REQUIRE(close(fd) == 0);
 }
 
+ATF_TC_WITHOUT_HEAD(mmap_prot);
+ATF_TC_BODY(mmap_prot, tc)
+{
+	void *p;
+	int fd, pagesize;
+
+	ATF_REQUIRE((pagesize = getpagesize()) > 0);
+
+	gen_test_path();
+	fd = shm_open(test_path, O_RDONLY | O_CREAT, 0644);
+	ATF_REQUIRE(fd >= 0);
+
+	p = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);
+	ATF_REQUIRE(p != MAP_FAILED);
+	ATF_REQUIRE(munmap(p, pagesize) == 0);
+	p = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	ATF_REQUIRE_ERRNO(EACCES, p == MAP_FAILED);
+	p = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+	ATF_REQUIRE(p != MAP_FAILED);
+	ATF_REQUIRE(munmap(p, pagesize) == 0);
+
+	ATF_REQUIRE_MSG(shm_unlink(test_path) == 0,
+	    "shm_unlink failed; errno=%d", errno);
+	ATF_REQUIRE_MSG(close(fd) == 0,
+	    "close failed; errno=%d", errno);
+}
+
 static int
 shm_open_large(int psind, int policy, size_t sz)
 {
@@ -1921,7 +1948,6 @@ ATF_TC_BODY(largepage_reopen, tc)
 
 ATF_TP_ADD_TCS(tp)
 {
-
 	ATF_TP_ADD_TC(tp, remap_object);
 	ATF_TP_ADD_TC(tp, rename_from_anon);
 	ATF_TP_ADD_TC(tp, rename_bad_path_pointer);
@@ -1955,6 +1981,7 @@ ATF_TP_ADD_TCS(tp)
 	ATF_TP_ADD_TC(tp, fallocate);
 	ATF_TP_ADD_TC(tp, fspacectl);
 	ATF_TP_ADD_TC(tp, accounting);
+	ATF_TP_ADD_TC(tp, mmap_prot);
 	ATF_TP_ADD_TC(tp, largepage_basic);
 	ATF_TP_ADD_TC(tp, largepage_config);
 	ATF_TP_ADD_TC(tp, largepage_mmap);
diff --git a/tests/sys/vm/mmap_test.c b/tests/sys/vm/mmap_test.c
index e5f4a81a7858..6bc30f73ca95 100644
--- a/tests/sys/vm/mmap_test.c
+++ b/tests/sys/vm/mmap_test.c
@@ -295,14 +295,82 @@ ATF_TC_BODY(mmap__write_only, tc)
 	munmap(p, pagesize);
 }
 
-ATF_TP_ADD_TCS(tp)
+ATF_TC_WITHOUT_HEAD(mmap__maxprot_basic);
+ATF_TC_BODY(mmap__maxprot_basic, tc)
+{
+	void *p;
+	int error, pagesize;
+
+	ATF_REQUIRE((pagesize = getpagesize()) > 0);
+
+	p = mmap(NULL, pagesize, PROT_READ | PROT_MAX(PROT_READ),
+	    MAP_ANON, -1, 0);
+	ATF_REQUIRE(p != MAP_FAILED);
+
+	error = mprotect(p, pagesize, PROT_WRITE);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+	error = mprotect(p, pagesize, PROT_READ | PROT_WRITE);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+	error = mprotect(p, pagesize, PROT_READ | PROT_EXEC);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+
+	ATF_REQUIRE(munmap(p, pagesize) == 0);
+}
+
+/* Make sure that PROT_MAX applies as expected to mappings of shm objects */
+ATF_TC_WITHOUT_HEAD(mmap__maxprot_shm);
+ATF_TC_BODY(mmap__maxprot_shm, tc)
 {
+	void *p;
+	int error, fd, pagesize;
+
+	ATF_REQUIRE((pagesize = getpagesize()) > 0);
 
+	fd = shm_open(SHM_ANON, O_RDWR, 0644);
+	ATF_REQUIRE(fd >= 0);
+
+	error = ftruncate(fd, pagesize);
+	ATF_REQUIRE(error == 0);
+
+	p = mmap(NULL, pagesize, PROT_READ | PROT_MAX(PROT_READ),
+	    MAP_PRIVATE, fd, 0);
+	ATF_REQUIRE(p != MAP_FAILED);
+
+	error = mprotect(p, pagesize, PROT_WRITE);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+	error = mprotect(p, pagesize, PROT_READ | PROT_WRITE);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+	error = mprotect(p, pagesize, PROT_READ | PROT_EXEC);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+
+	ATF_REQUIRE(munmap(p, pagesize) == 0);
+
+	/* Again, this time with a shared mapping. */
+	p = mmap(NULL, pagesize, PROT_READ | PROT_MAX(PROT_READ),
+	    MAP_SHARED, fd, 0);
+	ATF_REQUIRE(p != MAP_FAILED);
+
+	error = mprotect(p, pagesize, PROT_WRITE);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+	error = mprotect(p, pagesize, PROT_READ | PROT_WRITE);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+	error = mprotect(p, pagesize, PROT_READ | PROT_EXEC);
+	ATF_REQUIRE_ERRNO(EACCES, error == -1);
+
+	ATF_REQUIRE(munmap(p, pagesize) == 0);
+
+	ATF_REQUIRE(close(fd) == 0);
+}
+
+ATF_TP_ADD_TCS(tp)
+{
 	ATF_TP_ADD_TC(tp, mmap__map_at_zero);
 	ATF_TP_ADD_TC(tp, mmap__bad_arguments);
 	ATF_TP_ADD_TC(tp, mmap__dev_zero_private);
 	ATF_TP_ADD_TC(tp, mmap__dev_zero_shared);
 	ATF_TP_ADD_TC(tp, mmap__write_only);
+	ATF_TP_ADD_TC(tp, mmap__maxprot_basic);
+	ATF_TP_ADD_TC(tp, mmap__maxprot_shm);
 
 	return (atf_no_error());
 }