[LibOS] Do not call test_user_memory on LibOS allocated memory

This commit additionally fixes bugs in shim_do_sendmmsg and shim_do_recvmmsg - `vlen` argument is the number of items in the array, not size (in bytes) of it, as previous code assumed.
2026-08-22 05:56:23 +00:00 · 2021-01-05 20:12:08 +01:00
parent 3cb2112b05
commit 303528131c
3 changed files with 128 additions and 57 deletions
@@ -990,20 +990,6 @@ static ssize_t do_sendmsg(int fd, struct iovec* bufs, int nbufs, int flags,
    if (hdl->type != TYPE_SOCK)
        goto out;

-    struct shim_sock_handle* sock = &hdl->info.sock;
-
-    ret = -EFAULT;
-    if (addr && test_user_memory((void*)addr, addrlen, false))
-        goto out;
-
-    if (!bufs || test_user_memory(bufs, sizeof(*bufs) * nbufs, false))
-        goto out;
-
-    for (int i = 0; i < nbufs; i++) {
-        if (!bufs[i].iov_base || test_user_memory(bufs[i].iov_base, bufs[i].iov_len, false))
-            goto out;
-    }
-
    if (flags & ~(MSG_NOSIGNAL | MSG_DONTWAIT)) {
        debug("sendmsg()/sendmmsg()/sendto(): unknown flag (only MSG_NOSIGNAL and MSG_DONTWAIT"
              " are supported).\n");
@@ -1011,6 +997,15 @@ static ssize_t do_sendmsg(int fd, struct iovec* bufs, int nbufs, int flags,
        goto out;
    }

+    struct shim_sock_handle* sock = &hdl->info.sock;
+
+    if (addr) {
+        if (addrlen < 0 || (size_t)addrlen < minimal_addrlen(sock->domain)) {
+            ret = -EINVAL;
+            goto out;
+        }
+    }
+
    lock(&hdl->lock);

    if (flags & MSG_DONTWAIT) {
@@ -1130,6 +1125,14 @@ out:

 ssize_t shim_do_sendto(int sockfd, const void* buf, size_t len, int flags,
                       const struct sockaddr* addr, int addrlen) {
+    if (addr && test_user_memory((void*)addr, addrlen, /*write=*/false)) {
+        return -EFAULT;
+    }
+
+    if (!buf || test_user_memory((void*)buf, len, /*write=*/false)) {
+        return -EFAULT;
+    }
+
    struct iovec iovbuf;
    iovbuf.iov_base = (void*)buf;
    iovbuf.iov_len  = len;
@@ -1137,17 +1140,63 @@ ssize_t shim_do_sendto(int sockfd, const void* buf, size_t len, int flags,
    return do_sendmsg(sockfd, &iovbuf, 1, flags, addr, addrlen);
 }

+static int check_msghdr(struct msghdr* msg, bool is_recv) {
+    if (msg->msg_namelen < 0) {
+        return -EINVAL;
+    }
+
+    if (test_user_memory(msg->msg_name, msg->msg_namelen, /*write=*/is_recv)) {
+        return -EFAULT;
+    }
+
+    size_t size;
+    if (__builtin_mul_overflow(sizeof(*msg->msg_iov), msg->msg_iovlen, &size)) {
+        return -EMSGSIZE;
+    }
+
+    if (test_user_memory(msg->msg_iov, size, /*write=*/false)) {
+        return -EFAULT;
+    }
+
+    struct iovec* bufs = msg->msg_iov;
+    for (size_t i = 0; i < msg->msg_iovlen; i++) {
+        if (test_user_memory(bufs[i].iov_base, bufs[i].iov_len, /*write=*/is_recv)) {
+            return -EFAULT;
+        }
+    }
+
+    return 0;
+}
+
 ssize_t shim_do_sendmsg(int sockfd, struct msghdr* msg, int flags) {
+    if (!msg || test_user_memory(msg, sizeof(*msg), /*write=*/false)) {
+        return -EFAULT;
+    }
+
+    int ret = check_msghdr(msg, /*is_recv=*/false);
+    if (ret < 0) {
+        return ret;
+    }
+
    return do_sendmsg(sockfd, msg->msg_iov, msg->msg_iovlen, flags, msg->msg_name,
                      msg->msg_namelen);
 }

 ssize_t shim_do_sendmmsg(int sockfd, struct mmsghdr* msg, unsigned int vlen, int flags) {
-    if (test_user_memory(msg, vlen, /*write=*/true))
+    if (test_user_memory(msg, sizeof(*msg) * vlen, /*write=*/true)) {
        return -EFAULT;
+    }
+    for (size_t i = 0; i < vlen; i++) {
+        struct msghdr* m = &msg[i].msg_hdr;
+
+        int ret = check_msghdr(m, /*is_recv=*/false);
+        if (ret < 0) {
+            return ret;
+        }
+    }

    ssize_t total = 0;
-    for (size_t i = 0; i * sizeof(struct mmsghdr) < vlen; i++) {
+    for (size_t i = 0; i < vlen; i++) {
        struct msghdr* m = &msg[i].msg_hdr;

        ssize_t bytes =
@@ -1178,32 +1227,14 @@ static ssize_t do_recvmsg(int fd, struct iovec* bufs, size_t nbufs, int flags,
    struct shim_sock_handle* sock = &hdl->info.sock;

    if (addr) {
-        ret = -EINVAL;
-        if (!addrlen || test_user_memory(addrlen, sizeof(*addrlen), /*write=*/true))
-            goto out;
-
-        if (*addrlen < 0 || (size_t)*addrlen < minimal_addrlen(sock->domain))
-            goto out;
-
-        if (test_user_memory(addr, *addrlen, /*write=*/true))
+        if (*addrlen < 0 || (size_t)*addrlen < minimal_addrlen(sock->domain)) {
+            ret = -EINVAL;
            goto out;
+        }
    }

-    size_t bufs_size;
-    if (__builtin_mul_overflow(sizeof(*bufs), nbufs, &bufs_size)) {
-        ret = -EMSGSIZE;
-        goto out;
-    }
-
-    ret = -EFAULT;
-    if (!bufs || test_user_memory(bufs, bufs_size, /*write=*/false))
-        goto out;
-
    size_t expected_size = 0;
    for (size_t i = 0; i < nbufs; i++) {
-        if (!bufs[i].iov_base || test_user_memory(bufs[i].iov_base, bufs[i].iov_len,
-                                                  /*write=*/true))
-            goto out;
        expected_size += bufs[i].iov_len;
    }

@@ -1413,6 +1444,24 @@ out:

 ssize_t shim_do_recvfrom(int sockfd, void* buf, size_t len, int flags, struct sockaddr* addr,
                         int* addrlen) {
+    if (addr) {
+        if (test_user_memory(addrlen, sizeof(*addrlen), /*write=*/true)) {
+            return -EFAULT;
+        }
+
+        if (*addrlen < 0) {
+            return -EINVAL;
+        }
+
+        if (test_user_memory(addr, *addrlen, /*write=*/true)) {
+            return -EFAULT;
+        }
+    }
+
+    if (test_user_memory(buf, len, /*write=*/true)) {
+        return -EFAULT;
+    }
+
    struct iovec iovbuf;
    iovbuf.iov_base = (void*)buf;
    iovbuf.iov_len  = len;
@@ -1421,16 +1470,33 @@ ssize_t shim_do_recvfrom(int sockfd, void* buf, size_t len, int flags, struct so
 }

 ssize_t shim_do_recvmsg(int sockfd, struct msghdr* msg, int flags) {
+    if (test_user_memory(msg, sizeof(*msg), /*write=*/true)) {
+        return -EFAULT;
+    }
+
+    int ret = check_msghdr(msg, /*is_recv=*/true);
+    if (ret < 0) {
+        return ret;
+    }
+
    return do_recvmsg(sockfd, msg->msg_iov, msg->msg_iovlen, flags, msg->msg_name,
                      &msg->msg_namelen);
 }

 ssize_t shim_do_recvmmsg(int sockfd, struct mmsghdr* msg, unsigned int vlen, int flags,
                         struct __kernel_timespec* timeout) {
-    if (test_user_memory(msg, vlen, /*write=*/true))
+    if (test_user_memory(msg, sizeof(*msg) * vlen, /*write=*/true))
        return -EFAULT;

-    ssize_t total = 0;
+    for (size_t i = 0; i < vlen; i++) {
+        struct msghdr* m = &msg[i].msg_hdr;
+
+        int ret = check_msghdr(m, /*is_recv=*/true);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
    // Issue # 753 - https://github.com/oscarlab/graphene/issues/753
    /* TODO(donporter): timeout properly. For now, explicitly return an error. */
    if (timeout) {
@@ -1438,7 +1504,8 @@ ssize_t shim_do_recvmmsg(int sockfd, struct mmsghdr* msg, unsigned int vlen, int
        return -EOPNOTSUPP;
    }

-    for (size_t i = 0; i * sizeof(struct mmsghdr) < vlen; i++) {
+    ssize_t total = 0;
+    for (size_t i = 0; i < vlen; i++) {
        struct msghdr* m = &msg[i].msg_hdr;

        ssize_t bytes =
@@ -93,20 +93,11 @@ static void remove_qnode_from_wait_queue(struct shim_thread_queue* qnode) {
    }
 }

-long shim_do_waitid(int which, pid_t id, siginfo_t* infop, int options, struct __kernel_rusage* ru) {
-    __UNUSED(ru);
-
-    if (options & ~(WNOHANG | WNOWAIT | WEXITED | WSTOPPED | WCONTINUED |
-                    __WNOTHREAD | __WCLONE | __WALL))
-        return -EINVAL;
-
+static long do_waitid(int which, pid_t id, siginfo_t* infop, int options) {
    if (options & __WALL) {
        options &= ~__WCLONE;
    }

-    if (!(options & (WEXITED | WSTOPPED | WCONTINUED)))
-        return -EINVAL;
-
    if (options & WSTOPPED) {
        debug("Ignoring unsupported WSTOPPED flag to wait4\n");
        options &= ~WSTOPPED;
@@ -125,9 +116,6 @@ long shim_do_waitid(int which, pid_t id, siginfo_t* infop, int options, struct _
    if (!(which == P_PGID || which == P_ALL || which == P_PID))
        return -EINVAL;

-    if (infop && test_user_memory(infop, sizeof(*infop), /*write=*/true))
-        return -EFAULT;
-
    long ret = 0;

    lock(&g_process.children_lock);
@@ -225,7 +213,25 @@ out:
    return ret;
 }

+long shim_do_waitid(int which, pid_t id, siginfo_t* infop, int options, struct __kernel_rusage* ru) {
+    __UNUSED(ru);
+
+    if (options & ~(WNOHANG | WNOWAIT | WEXITED | WSTOPPED | WCONTINUED |
+                    __WNOTHREAD | __WCLONE | __WALL))
+        return -EINVAL;
+
+    if (!(options & (WEXITED | WSTOPPED | WCONTINUED)))
+        return -EINVAL;
+
+    if (infop && test_user_memory(infop, sizeof(*infop), /*write=*/true))
+        return -EFAULT;
+
+    return do_waitid(which, id, infop, options);
+}
+
 long shim_do_wait4(pid_t pid, int* status, int options, struct __kernel_rusage* ru) {
+    __UNUSED(ru);
+
    int which;
    pid_t id;
    siginfo_t info;
@@ -237,7 +243,7 @@ long shim_do_wait4(pid_t pid, int* status, int options, struct __kernel_rusage*
    if (status && test_user_memory(status, sizeof(*status), /*write=*/true))
        return -EFAULT;

-    /* Prepare options for shim_do_waitid(). */
+    /* Prepare options for do_waitid(). */
    options |= WEXITED;
    if (options & WUNTRACED) {
        options &= ~WUNTRACED;
@@ -259,7 +265,7 @@ long shim_do_wait4(pid_t pid, int* status, int options, struct __kernel_rusage*
    }

    info.si_pid = 0;
-    int ret = shim_do_waitid(which, id, &info, options, ru);
+    int ret = do_waitid(which, id, &info, options);
    if (ret < 0)
        return ret;

@@ -2055,8 +2055,6 @@ skip = yes
 skip = yes

 # EINVAL from native sendmsg()
-# also, shim_sendmmsg() has wrong vlen handling?
-# issue: https://github.com/oscarlab/graphene/issues/1931
 [sendmmsg01]
 skip = yes