On 4/1/22 4:08 PM, Daniel Verkamp wrote:

Basic tests to ensure that user/group/other execute bits cannot be changed after applying F_SEAL_EXEC to a memfd.

Signed-off-by: Daniel Verkamp dverkamp@chromium.org

tools/testing/selftests/memfd/memfd_test.c | 80 ++++++++++++++++++++++ 1 file changed, 80 insertions(+)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c index 94df2692e6e4..fdb0e46e9df9 100644 --- a/tools/testing/selftests/memfd/memfd_test.c +++ b/tools/testing/selftests/memfd/memfd_test.c @@ -28,6 +28,10 @@ #define MFD_DEF_SIZE 8192 #define STACK_SIZE 65536 +#ifndef F_SEAL_EXEC +#define F_SEAL_EXEC 0x0020 +#endif

• /*
  • Default is not to test hugetlbfs
  */

@@ -594,6 +598,48 @@ static void mfd_fail_grow_write(int fd) } } +static void mfd_assert_mode(int fd, int mode) +{

• struct stat st;
• if (fstat(fd, &st) < 0) {

• printf("fstat(%d) failed: %m\n", fd);
  

Let's print the filename here - just printing fd isn't useful.

• abort();
  

• } else if ((st.st_mode & 07777) != mode) {

• printf("wrong file mode 0%04o, but expected 0%04o\n",
  

•        (int)st.st_mode & 07777, mode);
  

This one doesn't even print fd - same comment here about filename.

• abort();
  

• }

+}

+static void mfd_assert_chmod(int fd, int mode) +{

• if (fchmod(fd, mode) < 0) {

• printf("fchmod(0%04o) failed: %m\n", mode);
  

Same here.

• abort();
  

• }
• mfd_assert_mode(fd, mode);

+}

+static void mfd_fail_chmod(int fd, int mode) +{

• struct stat st;
• if (fstat(fd, &st) < 0) {

• printf("fstat(%d) failed: %m\n", fd);
  

Same comment about filename

• abort();
  

• }
• if (fchmod(fd, mode) == 0) {

• printf("fchmod(0%04o) didn't fail as expected\n");
  

Same comment about filename

• abort();
  

• }
• /* verify that file mode bits did not change */
• mfd_assert_mode(fd, st.st_mode & 07777);

+}

• static int idle_thread_fn(void *arg) { sigset_t set;

@@ -880,6 +926,39 @@ static void test_seal_resize(void) close(fd); } +/*

• • Test SEAL_EXEC
• • Test that chmod() cannot change x bits after sealing
• */

+static void test_seal_exec(void) +{

• int fd;
• printf("%s SEAL-EXEC\n", memfd_str);
• fd = mfd_assert_new("kern_memfd_seal_exec",

• 	    mfd_def_size,
  

• 	    MFD_CLOEXEC | MFD_ALLOW_SEALING);
  

• mfd_assert_mode(fd, 0777);
• mfd_assert_chmod(fd, 0644);
• mfd_assert_has_seals(fd, 0);
• mfd_assert_add_seals(fd, F_SEAL_EXEC);
• mfd_assert_has_seals(fd, F_SEAL_EXEC);
• mfd_assert_chmod(fd, 0600);
• mfd_fail_chmod(fd, 0777);
• mfd_fail_chmod(fd, 0670);
• mfd_fail_chmod(fd, 0605);
• mfd_fail_chmod(fd, 0700);
• mfd_fail_chmod(fd, 0100);
• mfd_assert_chmod(fd, 0666);
• close(fd);

+}

• /*
  • Test sharing via dup()
  • Test that seals are shared between dupped FDs and they're all equal.

@@ -1059,6 +1138,7 @@ int main(int argc, char **argv) test_seal_shrink(); test_seal_grow(); test_seal_resize();

• test_seal_exec();

test_share_dup("SHARE-DUP", ""); test_share_mmap("SHARE-MMAP", "");

The rest looks good.

thanks, -- Shuah

From: Daniel Verkamp dverkamp@chromium.org

Tests that ensure MFD_NOEXEC memfds have the appropriate mode bits and cannot be chmod-ed into being executable.

Co-developed-by: Jeff Xu jeffxu@google.com Signed-off-by: Jeff Xu jeffxu@google.com Signed-off-by: Daniel Verkamp dverkamp@chromium.org --- tools/testing/selftests/memfd/memfd_test.c | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c index 1d7e7b36bbdd..4906f778564e 100644 --- a/tools/testing/selftests/memfd/memfd_test.c +++ b/tools/testing/selftests/memfd/memfd_test.c @@ -36,6 +36,10 @@ #define MAX_PATH 256 #endif

+#ifndef MFD_NOEXEC +#define MFD_NOEXEC 0x0008U +#endif + /* * Default is not to test hugetlbfs */ @@ -1006,6 +1010,35 @@ static void test_seal_exec(void) close(fd); }

+/* + * Test memfd_create with MFD_NOEXEC flag + * Test that MFD_NOEXEC applies F_SEAL_EXEC and prevents change of exec bits + */ +static void test_noexec(void) +{ + int fd; + + printf("%s NOEXEC\n", memfd_str); + + /* Create with NOEXEC and ALLOW_SEALING */ + fd = mfd_assert_new("kern_memfd_noexec", + mfd_def_size, + MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC); + mfd_assert_mode(fd, 0666); + mfd_assert_has_seals(fd, F_SEAL_EXEC); + mfd_fail_chmod(fd, 0777); + close(fd); + + /* Create with NOEXEC but without ALLOW_SEALING */ + fd = mfd_assert_new("kern_memfd_noexec", + mfd_def_size, + MFD_CLOEXEC | MFD_NOEXEC); + mfd_assert_mode(fd, 0666); + mfd_assert_has_seals(fd, F_SEAL_EXEC | F_SEAL_SEAL); + mfd_fail_chmod(fd, 0777); + close(fd); +} + /* * Test sharing via dup() * Test that seals are shared between dupped FDs and they're all equal. @@ -1179,6 +1212,7 @@ int main(int argc, char **argv)

test_create(); test_basic(); + test_noexec();

test_seal_write(); test_seal_future_write();

Hi Shuah Khan

I will continue Daniel Verkamp's work on this patch.

Could you please take a look at the new patch set I sent to see if all your comments are addressed ?

Much appreciated.

Best Regards, Jeff.

On Thu, Jul 28, 2022 at 11:15 PM Jeff Xu jeffxu@google.com wrote:

From: Daniel Verkamp dverkamp@chromium.org

Basic tests to ensure that user/group/other execute bits cannot be changed after applying F_SEAL_EXEC to a memfd.

Co-developed-by: Jeff Xu jeffxu@google.com Signed-off-by: Jeff Xu jeffxu@google.com Signed-off-by: Daniel Verkamp dverkamp@chromium.org

---

tools/testing/selftests/memfd/memfd_test.c | 129 ++++++++++++++++++++- 1 file changed, 128 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c index 94df2692e6e4..1d7e7b36bbdd 100644 --- a/tools/testing/selftests/memfd/memfd_test.c +++ b/tools/testing/selftests/memfd/memfd_test.c @@ -28,12 +28,44 @@ #define MFD_DEF_SIZE 8192 #define STACK_SIZE 65536

+#ifndef F_SEAL_EXEC +#define F_SEAL_EXEC 0x0020 +#endif

+#ifndef MAX_PATH +#define MAX_PATH 256 +#endif

/*

• Default is not to test hugetlbfs

*/ static size_t mfd_def_size = MFD_DEF_SIZE; static const char *memfd_str = MEMFD_STR;

+static ssize_t fd2name(int fd, char *buf, size_t bufsize) +{

•   char buf1[MAX_PATH];
  

•   int size;
  

•   ssize_t nbytes;
  

•   size = snprintf(buf1, MAX_PATH, "/proc/self/fd/%d", fd);
  

•   if (size < 0) {
  

•           printf("snprintf(%d) failed on %m\n", fd);
  

•           abort();
  

•   }
  

•   /*
  

•    * reserver one byte for string termination.
  

•    */
  

•   nbytes = readlink(buf1, buf, bufsize-1);
  

•   if (nbytes == -1) {
  

•           printf("readlink(%s) failed %m\n", buf1);
  

•           abort();
  

•   }
  

•   buf[nbytes] = '\0';
  

•   return nbytes;
  

+}

static int mfd_assert_new(const char *name, loff_t sz, unsigned int flags) { int r, fd; @@ -98,11 +130,14 @@ static unsigned int mfd_assert_get_seals(int fd)

static void mfd_assert_has_seals(int fd, unsigned int seals) {

•   char buf[MAX_PATH];
  

•   int nbytes;
    unsigned int s;
  

•   fd2name(fd, buf, MAX_PATH);
  
    s = mfd_assert_get_seals(fd);
    if (s != seals) {
  

•           printf("%u != %u = GET_SEALS(%d)\n", seals, s, fd);
  

•           printf("%u != %u = GET_SEALS(%s)\n", seals, s, buf);
            abort();
    }
  

} @@ -594,6 +629,64 @@ static void mfd_fail_grow_write(int fd) } }

+static void mfd_assert_mode(int fd, int mode) +{

•   struct stat st;
  

•   char buf[MAX_PATH];
  

•   int nbytes;
  

•   fd2name(fd, buf, MAX_PATH);
  

•   if (fstat(fd, &st) < 0) {
  

•           printf("fstat(%s) failed: %m\n", buf);
  

•           abort();
  

•   }
  

•   if ((st.st_mode & 07777) != mode) {
  

•           printf("fstat(%s) wrong file mode 0%04o, but expected 0%04o\n",
  

•                  buf, (int)st.st_mode & 07777, mode);
  

•           abort();
  

•   }
  

+}

+static void mfd_assert_chmod(int fd, int mode) +{

•   char buf[MAX_PATH];
  

•   int nbytes;
  

•   fd2name(fd, buf, MAX_PATH);
  

•   if (fchmod(fd, mode) < 0) {
  

•           printf("fchmod(%s, 0%04o) failed: %m\n", buf, mode);
  

•           abort();
  

•   }
  

•   mfd_assert_mode(fd, mode);
  

+}

+static void mfd_fail_chmod(int fd, int mode) +{

•   struct stat st;
  

•   char buf[MAX_PATH];
  

•   int nbytes;
  

•   fd2name(fd, buf, MAX_PATH);
  

•   if (fstat(fd, &st) < 0) {
  

•           printf("fstat(%s) failed: %m\n", buf);
  

•           abort();
  

•   }
  

•   if (fchmod(fd, mode) == 0) {
  

•           printf("fchmod(%s, 0%04o) didn't fail as expected\n",
  

•                  buf, mode);
  

•           abort();
  

•   }
  

•   /* verify that file mode bits did not change */
  

•   mfd_assert_mode(fd, st.st_mode & 07777);
  

+}

static int idle_thread_fn(void *arg) { sigset_t set; @@ -880,6 +973,39 @@ static void test_seal_resize(void) close(fd); }

+/*

• • Test SEAL_EXEC
• • Test that chmod() cannot change x bits after sealing
• */

+static void test_seal_exec(void) +{

•   int fd;
  

•   printf("%s SEAL-EXEC\n", memfd_str);
  

•   fd = mfd_assert_new("kern_memfd_seal_exec",
  

•                       mfd_def_size,
  

•                       MFD_CLOEXEC | MFD_ALLOW_SEALING);
  

•   mfd_assert_mode(fd, 0777);
  

•   mfd_assert_chmod(fd, 0644);
  

•   mfd_assert_has_seals(fd, 0);
  

•   mfd_assert_add_seals(fd, F_SEAL_EXEC);
  

•   mfd_assert_has_seals(fd, F_SEAL_EXEC);
  

•   mfd_assert_chmod(fd, 0600);
  

•   mfd_fail_chmod(fd, 0777);
  

•   mfd_fail_chmod(fd, 0670);
  

•   mfd_fail_chmod(fd, 0605);
  

•   mfd_fail_chmod(fd, 0700);
  

•   mfd_fail_chmod(fd, 0100);
  

•   mfd_assert_chmod(fd, 0666);
  

•   close(fd);
  

+}

/*

• Test sharing via dup()
• Test that seals are shared between dupped FDs and they're all equal.

@@ -1059,6 +1185,7 @@ int main(int argc, char **argv) test_seal_shrink(); test_seal_grow(); test_seal_resize();

•   test_seal_exec();
  
    test_share_dup("SHARE-DUP", "");
    test_share_mmap("SHARE-MMAP", "");
  

-- 2.37.1.455.g008518b4e5-goog

On 4/1/22 4:08 PM, Daniel Verkamp wrote:

Tests that ensure MFD_NOEXEC memfds have the appropriate mode bits and cannot be chmod-ed into being executable.

Signed-off-by: Daniel Verkamp dverkamp@chromium.org

tools/testing/selftests/memfd/memfd_test.c | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c index fdb0e46e9df9..a79567161cdf 100644 --- a/tools/testing/selftests/memfd/memfd_test.c +++ b/tools/testing/selftests/memfd/memfd_test.c @@ -32,6 +32,10 @@ #define F_SEAL_EXEC 0x0020 #endif +#ifndef MFD_NOEXEC +#define MFD_NOEXEC 0x0008U +#endif

• /*
  • Default is not to test hugetlbfs
  */

@@ -959,6 +963,35 @@ static void test_seal_exec(void) close(fd); } +/*

• • Test memfd_create with MFD_NOEXEC flag
• • Test that MFD_NOEXEC applies F_SEAL_EXEC and prevents change of exec bits
• */

+static void test_noexec(void) +{

• int fd;
• printf("%s NOEXEC\n", memfd_str);
• /* Create with NOEXEC and ALLOW_SEALING */
• fd = mfd_assert_new("kern_memfd_noexec",

• 	    mfd_def_size,
  

• 	    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC);
  

Don't we need to check fd here?

• mfd_assert_mode(fd, 0666);
• mfd_assert_has_seals(fd, F_SEAL_EXEC);
• mfd_fail_chmod(fd, 0777);
• close(fd);
• /* Create with NOEXEC but without ALLOW_SEALING */
• fd = mfd_assert_new("kern_memfd_noexec",

• 	    mfd_def_size,
  

• 	    MFD_CLOEXEC | MFD_NOEXEC);
  

What happens when mfd_assert_new() fails - don't we need to check fd?

• mfd_assert_mode(fd, 0666);
• mfd_assert_has_seals(fd, F_SEAL_EXEC | F_SEAL_SEAL);
• mfd_fail_chmod(fd, 0777);
• close(fd);

+}

• /*
  • Test sharing via dup()
  • Test that seals are shared between dupped FDs and they're all equal.

@@ -1132,6 +1165,7 @@ int main(int argc, char **argv) test_create(); test_basic();

• test_noexec();

test_seal_write(); test_seal_future_write();

fd isn't checked in the other test F_SEAL_EXEC in the 3/4 patch.

thanks, -- Shuah