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;
> +}
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
