On Tue, 11 Oct 2022 at 16:02, Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> 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
>
Thanks,
I thought I had sent the new revision, but it seems I have missed that.
>
> --
> Ondrej Mosnacek
> Senior Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
>
