[LibOS,PAL] Add flexible device-specific IOCTL support

The newly added `ioctl()` syscall emulation on device-backed file
descriptors is pass-through. It is insecure by itself since the
emulation only passes the arguments to and from the untrusted memory:
- IOCTL arguments are passed as-is from the app to the untrusted host,
  which may lead to leaks of secret data;
- untrusted host can change IOCTL arguments as it wishes when passing
  them from Gramine to the device and back.

It is the responsibility of the app developer to correctly use IOCTLs,
with security implications in mind. In most cases, IOCTL arguments should
be encrypted or integrity-protected with a key pre-shared between
Gramine and the device.

On the Linux-SGX PAL, a set of IOCTL requests must be explicitly allowed
in the manifest via the new option `sgx.allowed_ioctls`.
Also, the allowed IOCTLs' arguments (typically pointers to complex
nested objects) must be explicitly described in the manifest via the new
option `sgx.ioctl_structs.[identifier]` and a corresponding reference in
`sgx.allowed_ioctls`; see docs for explanation of the IOCTL struct
format.

This commit adds three new LibOS tests to verify the flexible IOCTL
logic against Gramine dummy device `/dev/gramine_test_dev`. This device
is located in companion repo `gramineproject/device-testing-tools`. One
test checks IOCTL data passing, second test checks that Gramine forbids
unknown IOCTLs, third test checks that Gramine IOCTL parser fails on
incorrectly defined IOCTL structs.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

.ci/lib/stage-build-sgx-vm.jenkinsfile

@@ -9,14 +9,17 @@ stage('build') {
         cd initramfs_builder
         {
            echo '#!/bin/sh'
-           echo 'if test -n $SGX; then GRAMINE=gramine-sgx; else GRAMINE=gramine-direct; fi'
+           echo 'set -e'
            echo 'cd $PWD_FOR_VM'
            echo '( cd device-testing-tools/gramine-device-testing-module; insmod gramine-testing-dev.ko )'
 
            # only couple tests -- executing in a VM with virtio-9p-pci FS passthrough is very slow
            echo 'cd libos/test/regression'
-           echo 'gramine-test build helloworld; $GRAMINE helloworld'
-           echo 'gramine-test build device_ioctl; $GRAMINE device_ioctl'
+           echo 'gramine-test build'
+           echo 'gramine-test pytest -v -k test_001_helloworld'
+           echo 'gramine-test pytest -v -k test_003_device_ioctl'
+           echo 'gramine-test pytest -v -k test_004_device_ioctl_fail'
+           echo 'gramine-test pytest -v -k test_005_device_ioctl_parse_fail'
            echo 'echo "TESTS OK"'
            echo 'poweroff -n -f'
         } > new_init

Documentation/manifest-syntax.rst

@@ -801,6 +801,147 @@ they were listed as ``allowed_files``. (However, this policy still does not
 allow writing/creating files specified as trusted.) This policy is a convenient
 way to determine the set of files that the ported application uses.
 
+Allowed IOCTLs
+^^^^^^^^^^^^^^
+
+::
+
+    sgx.ioctl_structs.[identifier] = [memory-layout-format]
+
+    sgx.allowed_ioctls = [
+      { request_code = [NUM], struct = "[identifier-of-ioctl-struct]" },
+    ]
+
+By default, Gramine disables all device-backed IOCTLs. This syntax allows to
+explicitly allow a set of IOCTLs on devices (devices must be explicitly mounted
+via ``fs.mounts`` manifest syntax). Only IOCTLs with the ``request_code``
+argument found among the manifest-listed IOCTLs are allowed to pass-through to
+the host. Each IOCTL entry may also contain a reference to an IOCTL struct in
+the ``struct`` field, in case the third IOCTL argument is intended to be
+translated by Gramine.
+
+Available IOCTL structs are described via ``sgx.ioctl_structs``. Each IOCTL
+struct describes the memory layout of the third argument to the ``ioctl`` system
+call (typically a pointer to a complex nested object passed to the device).
+Description of the memory layout is required for a deep copy of the IOCTL
+struct. Here we use the term *memory region* to denote a separate contiguous
+region of memory and the term *sub-region of a memory region* to denote a part
+of the memory region that has properties different from other sub-regions in the
+same memory region (e.g., should it be copied in or out of the SGX enclave, is
+it a pointer to another memory region, etc.). For example, a C struct can be
+considered one memory region, and fields of this C struct can be considered
+sub-regions of this memory region.
+
+Memory layout of the IOCTL struct is described using the TOML syntax of inline
+arrays (for each new separate memory region) and inline tables (for each
+sub-region in one memory region). Each sub-region is described via the following
+keys:
+
+- ``name`` is an optional name for this sub-region; mainly used to find
+  length-specifying sub-regions and nested memory regions.
+- ``alignment`` is an optional alignment of the memory region; may be specified
+  only in the first sub-region of a memory region (all other sub-regions are
+  contiguous with the first sub-region, so specifying their alignment doesn't
+  make sense). The default value is ``1``.
+- ``size`` is the size of this sub-region. The ``size`` field may be a string
+  with the name of another sub-region that contains the size value or an integer
+  with the constant size measured in ``unit`` units (default unit is 1 byte;
+  also see below). For example, ``size = "strlen"`` denotes a size field that
+  will be calculated dynamically during IOCTL execution based on the sub-region
+  named ``strlen``, whereas ``size = 16`` denotes a sub-region of size 16B. Note
+  that ``ptr`` sub-regions must *not* specify the ``size`` field.
+- ``unit`` is an optional unit of measurement for ``size``. It is 1 byte by
+  default. Unit of measurement must be a constant integer. For example,
+  ``size = "strlen"`` and ``unit = 2`` denote a wide-char string (where each
+  character is 2B long) of a dynamically specified length.
+- ``adjustment`` is an optional integer adjustment for ``size`` (always
+  specified in bytes). It is 0 bytes by default. This field must be a constant
+  (possibly negative) integer. For example, ``size = 6``, ``unit = 2`` and
+  ``adjustment = -8`` results in a total size of 4B.
+- ``direction = "none" | "out" | "in" | "inout"`` is an optional direction of
+  copy for this sub-region (from the app point of view). For example,
+  ``direction = "out"`` denotes a sub-region to be copied out of the enclave to
+  untrusted memory, i.e., this sub-region is an input to the host device. The
+  default value is ``none`` which is useful for e.g. padding of structs. This
+  field must be ommitted if the ``ptr`` field is specified for this sub-region
+  (pointer sub-regions contain the pointer value which will be unconditionally
+  rewired to point to untrusted memory).
+- ``ptr = inlined-memory-region`` or ``ptr = "another-ioctl-struct"``
+  specifies a pointer to another, nested memory region. This field is required
+  when describing complex IOCTL structs. Such pointer memory region always has
+  the implicit size of 8B, and the pointer value is always rewired by Gramine to
+  the memory region in untrusted memory (containing a corresponding nested
+  memory region). If ``ptr`` is specified together with ``array_len``, it
+  describes an array of pointers to these memory regions. (In other words,
+  ``ptr`` is an array of pointers to memory regions with ``array_len = 1`` by
+  default.) This may be recursive with the ``NULL`` value being a guard, which
+  allows describing linked lists.
+- ``array_len`` is an optional number of items in the ``ptr`` array. This field
+  cannot be specified with non-``ptr`` sub-regions. The default value is ``1``.
+
+Consider this simple C snippet:
+
+.. code-block:: c
+
+   struct ioctl_read {
+       size_t buf_size;                /* copied from enclave to device */
+       char* buf;                      /* copied from device to enclave */
+   } __attribute__((aligned(0x1000))); /* alignment just for illustration */
+
+This translates into the following manifest syntax::
+
+    sgx.ioctl_structs.ioctl_read = [
+        {
+            name      = "buf_size",
+            size      = 8,
+            direction = "out",
+            alignment = 0x1000
+        },
+        {
+            ptr = [
+                {
+                    size      = "buf_size",
+                    direction = "in"
+                }
+            ]
+        }
+    ]
+
+The above example specifies a root struct (first memory region) that consists of
+two sub-regions: the first one contains an 8-byte size value, the second one is
+an 8-byte pointer value. This pointer points to another memory region in enclave
+memory that contains a single sub-region of size ``buf_size``. This nested
+sub-region is copied from the device into the enclave.
+
+IOCTLs that use the above struct in a third argument are defined like this::
+
+    sgx.allowed_ioctls = [
+      { request_code = 0x12345678, struct = "ioctl_read" },
+      { request_code = 0x87654321, struct = "ioctl_read" },
+    ]
+
+If the IOCTL's third argument should be passed directly as-is (or unused at
+all), then the ``struct`` key must be an empty string or not exist at all::
+
+    sgx.allowed_ioctls = [
+      { request_code = 0x43218765, struct = "" },
+      { request_code = 0x87654321 },
+    ]
+
+.. note ::
+   IOCTLs for device communication are pass-through and thus potentially
+   insecure by themselves in SGX environments:
+
+   - IOCTL arguments are passed as-is from the app to the untrusted host,
+     which may lead to leaks of enclave data.
+   - Untrusted host can change IOCTL arguments as it wishes when passing
+     them from Gramine to the device and back.
+
+   It is the responsibility of the app developer to correctly use IOCTLs, with
+   security implications in mind. In most cases, IOCTL arguments should be
+   encrypted or integrity-protected with a key pre-shared between Gramine and
+   the device.
+
 Attestation and quotes
 ^^^^^^^^^^^^^^^^^^^^^^
 

libos/src/sys/libos_ioctl.c

@@ -13,6 +13,7 @@
 #include "linux_abi/errors.h"
 #include "linux_abi/ioctl.h"
 #include "pal.h"
+#include "stat.h"
 
 static void signal_io(IDTYPE caller, void* arg) {
     __UNUSED(caller);
@@ -30,14 +31,33 @@ static void signal_io(IDTYPE caller, void* arg) {
 }
 
 long libos_syscall_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg) {
+    int ret;
+
     struct libos_handle_map* handle_map = get_thread_handle_map(NULL);
     assert(handle_map);
 
     struct libos_handle* hdl = get_fd_handle(fd, NULL, handle_map);
     if (!hdl)
         return -EBADF;
 
-    int ret;
+    lock(&g_dcache_lock);
+    bool is_host_dev = hdl->type == TYPE_CHROOT && hdl->dentry->inode &&
+        hdl->dentry->inode->type == S_IFCHR;
+    unlock(&g_dcache_lock);
+
+    if (is_host_dev) {
+        int cmd_ret;
+        ret = PalDeviceIoControl(hdl->pal_handle, cmd, arg, &cmd_ret);
+        if (ret < 0) {
+            ret = pal_to_unix_errno(ret);
+            goto out;
+        }
+
+        assert(ret == 0);
+        ret = cmd_ret;
+        goto out;
+    }
+
     switch (cmd) {
         case TIOCGPGRP:
             if (!hdl->uri || strcmp(hdl->uri, "dev:tty") != 0) {
@@ -136,6 +156,7 @@ long libos_syscall_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg) {
             break;
     }
 
+out:
     put_handle(hdl);
     if (ret == -EINTR) {
         ret = -ERESTARTSYS;

libos/test/regression/device_ioctl.c

@@ -9,17 +9,133 @@
 #include "common.h"
 #include "rw_file.h"
 
-#include "gramine_test_dev_ioctl.h" /* currently unused */
+#include "gramine_test_dev_ioctl.h"
 
-#define STRING_READWRITE "Hello world via read/write\n"
+#define STRING_READWRITE      "Hello world via read/write\n"
+#define STRING_IOCTL          "Hello world via ioctls\n"
+#define STRING_IOCTL_REPLACED "He$$0 w0r$d via i0ct$s\n"
 
 int main(void) {
+    ssize_t bytes;
+    char buf[64];
+
     int devfd = CHECK(open("/dev/gramine_test_dev", O_RDWR));
 
-    ssize_t bytes = posix_fd_write(devfd, STRING_READWRITE, sizeof(STRING_READWRITE));
+    /* test 1 -- use write() and read() syscalls */
+    bytes = posix_fd_write(devfd, STRING_READWRITE, sizeof(STRING_READWRITE));
+    if (bytes != sizeof(STRING_READWRITE))
+        CHECK(-1);
+
+    /* lseek() doesn't work in Gramine because it is fully emulated in LibOS and therefore lseek()
+     * is not aware of device-specific semantics; instead we use a device-specific ioctl() */
+    off_t offset = CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_REWIND));
+    if (offset != 0)
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_REWIND) didn't return 0 "
+                "(returned: %ld)", offset);
+
+    memset(&buf, 0, sizeof(buf));
+    bytes = posix_fd_read(devfd, buf, sizeof(buf) - 1);
     if (bytes != sizeof(STRING_READWRITE))
         CHECK(-1);
 
+    if (strcmp(buf, STRING_READWRITE))
+        errx(1, "read `%s` from /dev/gramine_test_dev but expected `%s`", buf, STRING_READWRITE);
+
+    ssize_t devfd_size = CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_GETSIZE));
+    if (devfd_size != sizeof(STRING_READWRITE))
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_GETSIZE) didn't return %lu "
+                "(returned: %ld)", sizeof(STRING_READWRITE), devfd_size);
+
+    /* test 2 -- use ioctl(GRAMINE_TEST_DEV_IOCTL_WRITE) and ioctl(GRAMINE_TEST_DEV_IOCTL_READ)
+     *           syscalls */
+    CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_CLEAR));
+
+    devfd_size = CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_GETSIZE));
+    if (devfd_size != 0)
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_GETSIZE) didn't return 0 "
+                "(returned: %ld)", devfd_size);
+
+    struct gramine_test_dev_ioctl_write write_arg = {
+        .buf_size = sizeof(STRING_IOCTL),
+        .buf      = STRING_IOCTL,
+        .off      = 0
+    };
+    CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_WRITE, &write_arg));
+    if (write_arg.off != sizeof(STRING_IOCTL))
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_WRITE) didn't update offset "
+                "to %lu (returned: %ld)", sizeof(STRING_IOCTL), write_arg.off);
+    if (write_arg.copied != sizeof(STRING_IOCTL))
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_WRITE) didn't copy %lu bytes "
+                "(returned: %ld)", sizeof(STRING_IOCTL), write_arg.copied);
+
+    memset(buf, 0, sizeof(buf));
+    struct gramine_test_dev_ioctl_read read_arg = {
+        .buf_size = sizeof(buf) - 1,
+        .buf      = buf,
+        .off      = 0
+    };
+    CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_READ, &read_arg));
+    if (read_arg.off != sizeof(STRING_IOCTL))
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_READ) didn't update offset "
+                "to %lu (returned: %ld)", sizeof(STRING_IOCTL), read_arg.off);
+    if (read_arg.copied != sizeof(STRING_IOCTL))
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_READ) didn't copy %lu bytes "
+                "(returned: %ld)", sizeof(STRING_IOCTL), read_arg.copied);
+
+    if (strcmp(buf, STRING_IOCTL))
+        errx(1, "read `%s` from /dev/gramine_test_dev but expected `%s`", buf, STRING_IOCTL);
+
+    devfd_size = CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_GETSIZE));
+    if (devfd_size != sizeof(STRING_IOCTL))
+        errx(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_GETSIZE) didn't return %lu "
+                "(returned: %ld)", sizeof(STRING_IOCTL), devfd_size);
+
+    /* test 3 -- use ioctl(GRAMINE_TEST_DEV_IOCTL_REPLACE_ARR) syscall with a complex struct as an
+     *           argument */
+    struct gramine_test_dev_ioctl_replace_char replace_chars[] = {
+        { .src = 'l', .dst = '$' },
+        { .src = 'o', .dst = '0' }
+    };
+    struct gramine_test_dev_ioctl_replace_arr replace_arr = {
+        .replacements_cnt = ARRAY_LEN(replace_chars),
+        .replacements_arr = replace_chars
+    };
+    CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_REPLACE_ARR, &replace_arr));
+
+    memset(buf, 0, sizeof(buf));
+    read_arg = (struct gramine_test_dev_ioctl_read){
+        .buf_size = sizeof(buf) - 1,
+        .buf      = buf,
+        .off      = 0
+    };
+    CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_READ, &read_arg));
+    if (strcmp(buf, STRING_IOCTL_REPLACED))
+        errx(1, "read `%s` from /dev/gramine_test_dev but expected `%s`", buf,
+             STRING_IOCTL_REPLACED);
+
+    /* test 4 -- use ioctl(GRAMINE_TEST_DEV_IOCTL_REPLACE_LIST) syscall with a complex struct as an
+     *           argument */
+    struct gramine_test_dev_ioctl_replace_list replace_list_2 = {
+        .replacement = { .src = '0', .dst = 'o' },
+        .next = NULL
+    };
+    struct gramine_test_dev_ioctl_replace_list replace_list = {
+        .replacement = { .src = '$', .dst = 'l' },
+        .next = &replace_list_2
+    };
+
+    CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_REPLACE_LIST, &replace_list));
+
+    memset(buf, 0, sizeof(buf));
+    read_arg = (struct gramine_test_dev_ioctl_read){
+        .buf_size = sizeof(buf) - 1,
+        .buf      = buf,
+        .off      = 0
+    };
+    CHECK(ioctl(devfd, GRAMINE_TEST_DEV_IOCTL_READ, &read_arg));
+    if (strcmp(buf, STRING_IOCTL))
+        errx(1, "read `%s` from /dev/gramine_test_dev but expected `%s`", buf, STRING_IOCTL);
+
     CHECK(close(devfd));
     puts("TEST OK");
     return 0;