From d268b22b25f4a911067b3779f06dce8355af6868 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Mon, 17 Jul 2023 11:48:50 +0200 Subject: [PATCH 1/4] basic/memfd: drop test for F_SEAL_SEAL With `F_SEAL_SEAL` a memfd can disable further sealing operations, effectively sealing the set of seals. Testing for it ensures that no further seals can be added, it never prevents seals from being dropped, since seals cannot be dropped, ever. Now testing for `F_SEAL_SEAL` makes sense if you want to ensure that some seals are *not* set. That is, you either test for the entire set of seals to match a local set, or you verify that a specific seal is not set. Neither is what we are doing, so it feels wrong requiring it to be set. By dropping the requirement for `F_SEAL_SEAL`, the same FD can be shared with other entities while retaining the ability to further restrict the set of seals later on (e.g., being able to mark a region as executable later on, and then adding `F_SEAL_EXEC`). --- src/basic/memfd-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/memfd-util.c b/src/basic/memfd-util.c index 8e6946642b..c4e36b0ad4 100644 --- a/src/basic/memfd-util.c +++ b/src/basic/memfd-util.c @@ -113,7 +113,7 @@ int memfd_get_sealed(int fd) { return -errno; /* We ignore F_SEAL_EXEC here to support older kernels. */ - return FLAGS_SET(r, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL); + return FLAGS_SET(r, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE); } int memfd_get_size(int fd, uint64_t *sz) { From e1007a928a18baad7726113c9f473dd8b17cc0fe Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Mon, 17 Jul 2023 12:01:18 +0200 Subject: [PATCH 2/4] basic/memfd: reduce default seals to historic set Rather than always setting all seals, make `memfd_set_seals()` employ the original set of seals, that is: SEAL+GROW+SHRINK+WRITE Historically, the memfd code was used with the out-of-tree memfd patches, which merely supported a single seal ("SEALED", which effectively was GROW+SHRINK+WRITE). When the code was adapted to the upstream memfd seals, it was extended to the full seal set. With more and more seals being added upstream, this because more problematic. In particular, it is unclear what the function really is meant to achieve. Instead of just adding all seals, the function is returned to its original purpose: seal the memfd so futher modifications to its content are prevented. --- src/basic/memfd-util.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/basic/memfd-util.c b/src/basic/memfd-util.c index c4e36b0ad4..e21514fa9e 100644 --- a/src/basic/memfd-util.c +++ b/src/basic/memfd-util.c @@ -92,15 +92,9 @@ int memfd_map(int fd, uint64_t offset, size_t size, void **p) { } int memfd_set_sealed(int fd) { - int r; - assert(fd >= 0); - r = RET_NERRNO(fcntl(fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_EXEC | F_SEAL_SEAL)); - if (r == -EINVAL) /* old kernel ? */ - r = RET_NERRNO(fcntl(fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL)); - - return r; + return RET_NERRNO(fcntl(fd, F_ADD_SEALS, F_SEAL_SEAL | F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE)); } int memfd_get_sealed(int fd) { From 4d903003715b160acf7bf4baeffee7829ff99f85 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Mon, 17 Jul 2023 12:16:01 +0200 Subject: [PATCH 3/4] basic/memfd: add fcntl() wrappers Add wrappers around GET/ADD_SEALS to allow future use outside of the current `memfd_get/set_sealed()` helpers. --- src/basic/memfd-util.c | 33 +++++++++++++++++++++++++-------- src/basic/memfd-util.h | 2 ++ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/basic/memfd-util.c b/src/basic/memfd-util.c index e21514fa9e..a80d586ffa 100644 --- a/src/basic/memfd-util.c +++ b/src/basic/memfd-util.c @@ -68,6 +68,26 @@ int memfd_new(const char *name) { return memfd_create_wrapper(name, MFD_ALLOW_SEALING | MFD_CLOEXEC | MFD_NOEXEC_SEAL); } +int memfd_add_seals(int fd, unsigned int seals) { + assert(fd >= 0); + + return RET_NERRNO(fcntl(fd, F_ADD_SEALS, seals)); +} + +int memfd_get_seals(int fd, unsigned int *ret_seals) { + int r; + + assert(fd >= 0); + + r = RET_NERRNO(fcntl(fd, F_GET_SEALS)); + if (r < 0) + return r; + + if (ret_seals) + *ret_seals = r; + return 0; +} + int memfd_map(int fd, uint64_t offset, size_t size, void **p) { void *q; int sealed; @@ -92,22 +112,19 @@ int memfd_map(int fd, uint64_t offset, size_t size, void **p) { } int memfd_set_sealed(int fd) { - assert(fd >= 0); - - return RET_NERRNO(fcntl(fd, F_ADD_SEALS, F_SEAL_SEAL | F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE)); + return memfd_add_seals(fd, F_SEAL_SEAL | F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE); } int memfd_get_sealed(int fd) { + unsigned int seals; int r; - assert(fd >= 0); - - r = fcntl(fd, F_GET_SEALS); + r = memfd_get_seals(fd, &seals); if (r < 0) - return -errno; + return r; /* We ignore F_SEAL_EXEC here to support older kernels. */ - return FLAGS_SET(r, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE); + return FLAGS_SET(seals, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE); } int memfd_get_size(int fd, uint64_t *sz) { diff --git a/src/basic/memfd-util.h b/src/basic/memfd-util.h index 0fe8e3a3c4..9b2103e0ce 100644 --- a/src/basic/memfd-util.h +++ b/src/basic/memfd-util.h @@ -12,6 +12,8 @@ int memfd_new(const char *name); int memfd_new_and_map(const char *name, size_t sz, void **p); int memfd_new_and_seal(const char *name, const void *data, size_t sz); +int memfd_add_seals(int fd, unsigned int seals); +int memfd_get_seals(int fd, unsigned int *ret_seals); int memfd_map(int fd, uint64_t offset, size_t size, void **p); int memfd_set_sealed(int fd); From 69688410566aa9e2a00530abd91e7dfef0212c83 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Mon, 17 Jul 2023 12:17:56 +0200 Subject: [PATCH 4/4] basic/memfd: fix memfd_map() seal test Private mappings are required when F_SEAL_WRITE is set on a memfd, because otherwise you could end up with writable mappings through mprotect() and other calls. This is a limitation of the kernel implementation, and might be lifted by future extensions. Regardless, the current code tests for the full `is_sealed()` before using MAP_PRIVATE. This might end up using MAP_SHARED for write-sealed memfds, which will be refused by the kernel. Fix this and make memfd_map() check for exactly `F_SEAL_WRITE`. --- src/basic/memfd-util.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/basic/memfd-util.c b/src/basic/memfd-util.c index a80d586ffa..92b84f95a6 100644 --- a/src/basic/memfd-util.c +++ b/src/basic/memfd-util.c @@ -89,18 +89,19 @@ int memfd_get_seals(int fd, unsigned int *ret_seals) { } int memfd_map(int fd, uint64_t offset, size_t size, void **p) { + unsigned int seals; void *q; - int sealed; + int r; assert(fd >= 0); assert(size > 0); assert(p); - sealed = memfd_get_sealed(fd); - if (sealed < 0) - return sealed; + r = memfd_get_seals(fd, &seals); + if (r < 0) + return r; - if (sealed) + if (seals & F_SEAL_WRITE) q = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, offset); else q = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset);