On Mon, Sep 12, 2022 at 3:41 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Wed, Aug 31, 2022 at 5:34 PM Christian Göttsche
> <cgzones@xxxxxxxxxxxxxx> wrote:
> > Add test for memfd_secret(2) anonymous inodes check added in 6.0 via
> > 2bfe15c52612 ("mm: create security context for memfd_secret inodes").
> >
> > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> > ---
> > v2:
> > - print mmap failures to stdout, since they are expected when
> > mapping with PROT_EXEC
> > ---
> > .github/workflows/checks.yml | 4 ++
> > Vagrantfile | 10 +++--
> > policy/Makefile | 4 ++
> > policy/test_secretmem.te | 33 ++++++++++++++
> > tests/Makefile | 5 +++
> > tests/secretmem/.gitignore | 1 +
> > tests/secretmem/Makefile | 5 +++
> > tests/secretmem/secretmem.c | 83 ++++++++++++++++++++++++++++++++++++
> > tests/secretmem/test | 39 +++++++++++++++++
> > 9 files changed, 180 insertions(+), 4 deletions(-)
> > create mode 100644 policy/test_secretmem.te
> > create mode 100644 tests/secretmem/.gitignore
> > create mode 100644 tests/secretmem/Makefile
> > create mode 100644 tests/secretmem/secretmem.c
> > create mode 100755 tests/secretmem/test
>
> This looks good to me, with some minor comments below.
>
> > diff --git a/tests/secretmem/secretmem.c b/tests/secretmem/secretmem.c
> > new file mode 100644
> > index 0000000..0d541ee
> > --- /dev/null
> > +++ b/tests/secretmem/secretmem.c
> > @@ -0,0 +1,83 @@
> > +#include <errno.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +#include <sys/mman.h>
> > +#include <sys/syscall.h>
> > +
> > +#ifndef __NR_memfd_secret
> > +# define __NR_memfd_secret 447
> > +#endif
> > +
> > +#define TEXT "Hello World!\nHello World!\nHello World!\nHello World!\nHello World!\nHello World!\n"
> > +
> > +static int _memfd_secret(unsigned long flags)
> > +{
> > + return syscall(__NR_memfd_secret, flags);
> > +}
> > +
> > +int main(int argc, const char *argv[])
> > +{
> > + long page_size;
> > + int fd, flags;
> > + char *mem;
> > + bool check = (argc == 2 && strcmp(argv[1], "check") == 0);
> > + bool wx = (argc == 2 && strcmp(argv[1], "wx") == 0);
> > +
> > + page_size = sysconf(_SC_PAGESIZE);
> > + if (page_size <= 0) {
> > + fprintf(stderr, "failed to get pagesize, got %ld: %s\n", page_size,
> > + strerror(errno));
> > + return EXIT_FAILURE;
> > + }
> > +
> > + fd = _memfd_secret(0);
> > + if (fd < 0) {
> > + printf("memfd_secret() failed: %s\n", strerror(errno));
> > + if (check && errno != ENOSYS)
> > + return EXIT_SUCCESS;
> > +
> > + return EXIT_FAILURE;
> > + }
> > +
> > + if (check)
> > + return EXIT_SUCCESS;
> > +
> > + if (ftruncate(fd, page_size) < 0) {
> > + fprintf(stderr, "ftruncate failed: %s\n", strerror(errno));
> > + }
> > +
> > + flags = PROT_READ | PROT_WRITE;
> > + if (wx)
> > + flags |= PROT_EXEC;
> > +
> > + mem = mmap(NULL, page_size, flags, MAP_SHARED, fd, 0);
> > + if (mem == MAP_FAILED || !mem) {
> > + printf("unable to mmap secret memory: %s\n", strerror(errno));
> > + close(fd);
> > + return EXIT_FAILURE;
> > + }
> > +
> > + close(fd);
> > +
> > + memcpy(mem, TEXT, sizeof TEXT);
>
> Please use parentheses with sizeof. When the argument is a type name
> they are mandatory so it's better to use them always for consistency.
> See also:
> https://lore.kernel.org/lkml/CA+55aFwey-q4716pYYSi=3R_ucw84zFspDXMXmzvzc72XSc9Lg@xxxxxxxxxxxxxx/
> https://lore.kernel.org/lkml/CA+55aFwcJgAFiow1sSo7mkF9n0MpTw80gjAszazyBrRcmbph-g@xxxxxxxxxxxxxx/
>
> > +
> > + if (memcmp(mem, TEXT, sizeof TEXT) != 0) {
> > + fprintf(stderr, "data not synced (1)\n");
> > + munmap(mem, page_size);
> > + return EXIT_FAILURE;
> > + }
> > +
> > + if (strlen(mem) + 1 != sizeof TEXT) {
> > + fprintf(stderr, "data not synced (2)\n");
> > + munmap(mem, page_size);
> > + return EXIT_FAILURE;
> > + }
>
> What is the point of this second check? The previous check already
> asserts that the contents are char-to-char equal to TEXT (including
> the terminating null character), which implies string length
> equivalence as well.
>
> > +
> > + munmap(mem, page_size);
> > +
> > + return EXIT_SUCCESS;
> > +}
You only pushed the updated version to the GitHub PR
(https://github.com/SELinuxProject/selinux-testsuite/pull/80), but
anyway I took that version and applied it (just removed the changelog
from the commit message and added my SOB):
https://github.com/SELinuxProject/selinux-testsuite/commit/77352e748f006c343d602e4be03ae0d2cfcca831
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
