From 51a16c84120280288e372241a29d9ab802cd0b95 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 14 Apr 2020 13:32:03 +0000 Subject: 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 --- sys/kern/uipc_shm.c | 16 +++++++++------- tests/sys/kern/memfd_test.c | 19 ++++++++++++++----- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/sys/kern/uipc_shm.c b/sys/kern/uipc_shm.c index a5f550f35e62..dc8789716f3d 100644 --- a/sys/kern/uipc_shm.c +++ b/sys/kern/uipc_shm.c @@ -1147,14 +1147,16 @@ shm_mmap(struct file *fp, vm_map_t map, vm_offset_t *addr, vm_size_t objsize, 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; } diff --git a/tests/sys/kern/memfd_test.c b/tests/sys/kern/memfd_test.c index 6ab4c2a14258..5d5b8f9e6084 100644 --- a/tests/sys/kern/memfd_test.c +++ b/tests/sys/kern/memfd_test.c @@ -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); @@ -260,7 +270,6 @@ ATF_TC_BODY(immutable_seals, tc) close(fd); } - ATF_TP_ADD_TCS(tp) { -- cgit v1.2.3