[PATCH 1/2] linux-user: validate the fd for a zero-length readv/writev

[PATCH 1/2] linux-user: validate the fd for a zero-length readv/writev

[TAKANO Mitsuhiro]

lock_iovec() returns NULL with errno left at zero when the vector count
is zero, and the readv/writev family turns that into a successful return
without ever entering the kernel.  The kernel checks the file descriptor
before it looks at the vector, so a read from a write-only descriptor
fails with EBADF even when iovcnt is zero, while qemu-user reports
success:

    fd = open("/dev/null", O_WRONLY);
    readv(fd, NULL, 0);     /* kernel: -1 EBADF, qemu-user: 0 */

Return an empty vector for the zero count so that the syscall is issued
and the host reports the real error.  This covers readv, writev,
preadv, pwritev, preadv2, pwritev2 and vmsplice, which all share
lock_iovec(); sendmsg/recvmsg already handle a zero iov itself.

Reproducer and measurements: https://github.com/takano32/qemu-user-zero-iovec

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 linux-user/syscall.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0380691a8b..3197ccaec6 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3093,8 +3093,13 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
     bool bad_address = false;
 
     if (count == 0) {
-        errno = 0;
-        return NULL;
+        /*
+         * The kernel validates the file descriptor before it looks at the
+         * vector, so readv(fd, NULL, 0) on a read-only fd fails with EBADF
+         * rather than returning 0.  Hand back an empty vector so that the
+         * caller issues the syscall and the host reports the real error.
+         */
+        return g_new0(struct iovec, 1);
     }
     if (count > IOV_MAX) {
         errno = EINVAL;
-- 
2.53.0

[PATCH 2/2] tests/tcg: check that a zero-length readv/writev validates the fd

[TAKANO Mitsuhiro]

The kernel checks the file mode before it looks at the vector, so an
empty vector on a descriptor opened the wrong way has to fail with
EBADF rather than report a successful no-op.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 .../multiarch/linux/linux-zero-len-iovec.c    | 41 +++++++++++++++++++
 tests/tcg/multiarch/meson.build               |  1 +
 2 files changed, 42 insertions(+)
 create mode 100644 tests/tcg/multiarch/linux/linux-zero-len-iovec.c

diff --git a/tests/tcg/multiarch/linux/linux-zero-len-iovec.c b/tests/tcg/multiarch/linux/linux-zero-len-iovec.c
new file mode 100644
index 0000000000..e16b643ed4
--- /dev/null
+++ b/tests/tcg/multiarch/linux/linux-zero-len-iovec.c
@@ -0,0 +1,41 @@
+/*
+ * A zero-length readv()/writev() still validates the file descriptor.
+ *
+ * The kernel checks the file mode before it looks at the vector, so an
+ * empty vector on a descriptor opened the wrong way fails with EBADF
+ * instead of returning 0.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#define _GNU_SOURCE
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <sys/uio.h>
+#include <unistd.h>
+
+int main(void)
+{
+    int ro = open("/dev/null", O_RDONLY);
+    int wo = open("/dev/null", O_WRONLY);
+
+    assert(ro >= 0);
+    assert(wo >= 0);
+
+    errno = 0;
+    assert(readv(wo, NULL, 0) == -1 && errno == EBADF);
+    errno = 0;
+    assert(writev(ro, NULL, 0) == -1 && errno == EBADF);
+
+    /* A closed descriptor is rejected all the same. */
+    close(wo);
+    errno = 0;
+    assert(readv(wo, NULL, 0) == -1 && errno == EBADF);
+
+    /* On a usable descriptor an empty vector is a successful no-op. */
+    assert(readv(ro, NULL, 0) == 0);
+
+    close(ro);
+    return EXIT_SUCCESS;
+}
diff --git a/tests/tcg/multiarch/meson.build b/tests/tcg/multiarch/meson.build
index e60a052700..97e2ba76dc 100644
--- a/tests/tcg/multiarch/meson.build
+++ b/tests/tcg/multiarch/meson.build
@@ -61,6 +61,7 @@ tests += {
   multiarch/'linux/linux-shmat-null.c': {},
   multiarch/'linux/linux-sigrtminmax.c': {},
   multiarch/'linux/linux-test.c': {},
+  multiarch/'linux/linux-zero-len-iovec.c': {},
   multiarch/'linux/test-vma.c': {},
 }
 
-- 
2.53.0

Re: [PATCH 1/2] linux-user: validate the fd for a zero-length readv/writev

[Alex Bennée]

TAKANO Mitsuhiro <takano32@gmail.com> writes:

> lock_iovec() returns NULL with errno left at zero when the vector count
> is zero, and the readv/writev family turns that into a successful return
> without ever entering the kernel.  The kernel checks the file descriptor
> before it looks at the vector, so a read from a write-only descriptor
> fails with EBADF even when iovcnt is zero, while qemu-user reports
> success:
>
>     fd = open("/dev/null", O_WRONLY);
>     readv(fd, NULL, 0);     /* kernel: -1 EBADF, qemu-user: 0 */
>
> Return an empty vector for the zero count so that the syscall is issued
> and the host reports the real error.  This covers readv, writev,
> preadv, pwritev, preadv2, pwritev2 and vmsplice, which all share
> lock_iovec(); sendmsg/recvmsg already handle a zero iov itself.
>
> Reproducer and measurements: https://github.com/takano32/qemu-user-zero-iovec
>
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
> ---
>  linux-user/syscall.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 0380691a8b..3197ccaec6 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3093,8 +3093,13 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
>      bool bad_address = false;
>  
>      if (count == 0) {
> -        errno = 0;
> -        return NULL;
> +        /*
> +         * The kernel validates the file descriptor before it looks at the
> +         * vector, so readv(fd, NULL, 0) on a read-only fd fails with EBADF
> +         * rather than returning 0.  Hand back an empty vector so that the
> +         * caller issues the syscall and the host reports the real error.
> +         */
> +        return g_new0(struct iovec, 1);

I can't imagine why we would want to leak an iovec structure every time
this is called. All the callers seem to have handling for NULL. Why are
we not just setting the right errno?

>      }
>      if (count > IOV_MAX) {
>          errno = EINVAL;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 0/2] linux-user: validate the fd for a zero-length readv/writev

[TAKANO Mitsuhiro]

Hi Alex,

thanks for the review.

> I can't imagine why we would want to leak an iovec structure every time
> this is called. All the callers seem to have handling for NULL. Why are
> we not just setting the right errno?

It is not leaked: every caller hands the vector to unlock_iovec(), which
g_free()s it, and with count == 0 the loop over the target vector does not
run.  But you are right that there is no reason to allocate at all, so v2
returns a shared, empty vector instead.  It is never dereferenced, and
unlock_iovec() now skips the free.

Setting errno in lock_iovec() does not work, though: the answer depends on
the descriptor, which lock_iovec() does not have.  readv(fd, NULL, 0) is a
successful no-op on a usable descriptor and EBADF on a wrong one, so the
syscall has to be issued for the host to answer.

Since "the kernel does X" is a strong claim, I checked both halves.

Kernel side, in this tree:

  - fs/read_write.c: vfs_readv()/vfs_writev() return -EBADF from the
    FMODE_READ/FMODE_WRITE test before import_iovec() is reached, so an
    empty vector still validates the descriptor.
  - net/socket.c: __sys_recvmsg() returns -EBADF from the fd lookup before
    the msghdr is parsed, so recvmsg() with iovlen 0 is affected too.
    do_sendrecvmsg() turned that zero count into a successful return for
    !send, which is why v2 covers recvmsg() in the test; sendmsg() already
    issued the syscall through the "allow sending packet without any iov"
    path.

Behaviour side.  I wrote a harness that calls readv, writev, preadv,
pwritev, preadv2, pwritev2, vmsplice, sendmsg and recvmsg with an empty
vector, plus readv/writev with a bogus vector pointer, on eleven fd states
(/dev/null opened O_RDONLY, O_WRONLY, O_RDWR and O_PATH, a directory, both
ends of a pipe, both ends of a datagram socketpair, one end of a stream
socketpair, and a closed fd).  That is 132 lines of output per run.

  - v2 matches the native kernel run line for line on aarch64, arm, i386,
    ppc (32-bit BE), s390x (64-bit BE) and x86_64.
  - unpatched 10.2.1 differs from the kernel in 64 of those 132 lines.
  - the only mismatch the harness still finds is readv(fd, NULL, -1),
    where qemu returns EINVAL and the kernel returns EBADF for a bad fd.
    That is the pre-existing "count > IOV_MAX is rejected before the fd is
    looked at" behaviour, which this series does not touch; doing it
    properly needs the fd in the caller, so it would be a separate change.

The regression comes from f287b2c2d4: before that commit lock_iovec()
succeeded for count == 0 (page_check_range() returns 0 for a zero length)
and the syscall was issued.  Hence the Fixes: tag.

Tests:
  - tests/tcg with the new test in the suite: aarch64 106/106, i386 78/78,
    arm 64/64, s390x 135/135, x86_64 73/73.
  - ppc-linux-user has no tests/tcg directory upstream, so I built the
    same test by hand with powerpc-linux-gnu-gcc: it passes with the
    series and aborts on 10.2.1.
  - the new test compiles warning-free with -Wall -Wextra -Werror for all
    six cross compilers.

Because the vector is shared now, I also checked memory and concurrency:
eight threads issuing 4.8 million empty readv/writev calls against valid
and wrong-mode descriptors finished with no failures, and qemu under
valgrind reports 0 errors and 0 bytes definitely lost.

Changes in v2:
  - return a shared empty vector instead of allocating one per call
  - explain in the comment why errno cannot be set here
  - cover recvmsg(), which had the same bug, in the test

If you think this corner is not worth carrying, say so and I will drop it
- the divergence is real but small.

Thanks,
Takano

TAKANO Mitsuhiro (2):
  linux-user: validate the fd for a zero-length readv/writev
  tests/tcg: check that a zero-length readv/writev validates the fd

 linux-user/syscall.c                          | 20 ++++++-
 .../multiarch/linux/linux-zero-len-iovec.c    | 56 +++++++++++++++++++
 tests/tcg/multiarch/meson.build               |  1 +
 3 files changed, 74 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/multiarch/linux/linux-zero-len-iovec.c

-- 
2.53.0

[PATCH v2 1/2] linux-user: validate the fd for a zero-length readv/writev

[TAKANO Mitsuhiro]

lock_iovec() returns NULL with errno left at zero when the vector count
is zero, and the readv/writev family turns that into a successful return
without ever entering the kernel.  The kernel checks the file descriptor
before it looks at the vector, so a read from a write-only descriptor
fails with EBADF even when iovcnt is zero, while qemu-user reports
success:

    fd = open("/dev/null", O_WRONLY);
    readv(fd, NULL, 0);     /* kernel: -1 EBADF, qemu-user: 0 */

errno cannot be set in lock_iovec() instead: an empty vector is a
successful no-op on a usable descriptor and only the descriptor decides,
so the syscall has to be issued for the host to answer.  Return a shared
empty vector, which is never dereferenced and which unlock_iovec() leaves
alone, so that every caller reaches the host.

This covers readv, writev, preadv, pwritev, preadv2, pwritev2, vmsplice
and recvmsg, which all share lock_iovec(); sendmsg already issued the
syscall for a zero-length vector.

Fixes: f287b2c2d4d2 ("linux-user: Perform more checks on iovec lists")

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 linux-user/syscall.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0380691a8b..75151c1054 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3082,6 +3082,13 @@ static void target_to_host_low_high(abi_ulong tlow,
     *hhigh = (off >> HOST_LONG_BITS / 2) >> HOST_LONG_BITS / 2;
 }
 
+/*
+ * A zero-length vector is handed to the host so that the syscall is still
+ * issued: the kernel validates the file descriptor before it looks at the
+ * vector.  Nothing is ever written to it.
+ */
+static struct iovec empty_iovec;
+
 static struct iovec *lock_iovec(int type, abi_ulong target_addr,
                                 abi_ulong count, int copy)
 {
@@ -3093,8 +3100,13 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
     bool bad_address = false;
 
     if (count == 0) {
-        errno = 0;
-        return NULL;
+        /*
+         * There is nothing to lock, but returning an error would skip the
+         * syscall: readv(fd, NULL, 0) on a write-only descriptor fails with
+         * EBADF in the kernel, and succeeds when the descriptor is fine, so
+         * only the host syscall can produce the right answer.
+         */
+        return &empty_iovec;
     }
     if (count > IOV_MAX) {
         errno = EINVAL;
@@ -3190,7 +3202,9 @@ static void unlock_iovec(struct iovec *vec, abi_ulong target_addr,
         unlock_user(target_vec, target_addr, 0);
     }
 
-    g_free(vec);
+    if (vec != &empty_iovec) {
+        g_free(vec);
+    }
 }
 
 static inline int target_to_host_sock_type(int *type)
-- 
2.53.0

[PATCH v2 2/2] tests/tcg: check that a zero-length readv/writev validates the fd

[TAKANO Mitsuhiro]

The kernel checks the file mode before it looks at the vector, so an
empty vector on a descriptor opened the wrong way has to fail with EBADF
rather than report a successful no-op.  Cover readv, writev and recvmsg,
including the usable descriptor cases, where the call has to return 0 or
the error of the socket instead.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 .../multiarch/linux/linux-zero-len-iovec.c    | 56 +++++++++++++++++++
 tests/tcg/multiarch/meson.build               |  1 +
 2 files changed, 57 insertions(+)
 create mode 100644 tests/tcg/multiarch/linux/linux-zero-len-iovec.c

diff --git a/tests/tcg/multiarch/linux/linux-zero-len-iovec.c b/tests/tcg/multiarch/linux/linux-zero-len-iovec.c
new file mode 100644
index 0000000000..79666dba88
--- /dev/null
+++ b/tests/tcg/multiarch/linux/linux-zero-len-iovec.c
@@ -0,0 +1,56 @@
+/*
+ * A zero-length readv()/writev()/recvmsg() still validates the file
+ * descriptor.
+ *
+ * The kernel checks the file mode before it looks at the vector, so an
+ * empty vector on a descriptor opened the wrong way fails with EBADF
+ * instead of returning 0.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#define _GNU_SOURCE
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <sys/socket.h>
+#include <sys/uio.h>
+#include <unistd.h>
+
+int main(void)
+{
+    int ro = open("/dev/null", O_RDONLY);
+    int wo = open("/dev/null", O_WRONLY);
+    struct msghdr msg = { 0 };
+    int sv[2];
+
+    assert(ro >= 0);
+    assert(wo >= 0);
+
+    errno = 0;
+    assert(readv(wo, NULL, 0) == -1 && errno == EBADF);
+    errno = 0;
+    assert(writev(ro, NULL, 0) == -1 && errno == EBADF);
+
+    /* A closed descriptor is rejected all the same. */
+    close(wo);
+    errno = 0;
+    assert(readv(wo, NULL, 0) == -1 && errno == EBADF);
+
+    /* On a usable descriptor an empty vector is a successful no-op. */
+    assert(readv(ro, NULL, 0) == 0);
+
+    /* recvmsg() with an empty vector reaches the socket too. */
+    assert(socketpair(AF_UNIX, SOCK_DGRAM, 0, sv) == 0);
+    close(sv[0]);
+    msg.msg_iov = NULL;
+    msg.msg_iovlen = 0;
+    errno = 0;
+    assert(recvmsg(sv[0], &msg, 0) == -1 && errno == EBADF);
+    errno = 0;
+    assert(recvmsg(sv[1], &msg, MSG_DONTWAIT) == -1 && errno == EAGAIN);
+
+    close(sv[1]);
+    close(ro);
+    return EXIT_SUCCESS;
+}
diff --git a/tests/tcg/multiarch/meson.build b/tests/tcg/multiarch/meson.build
index e60a052700..97e2ba76dc 100644
--- a/tests/tcg/multiarch/meson.build
+++ b/tests/tcg/multiarch/meson.build
@@ -61,6 +61,7 @@ tests += {
   multiarch/'linux/linux-shmat-null.c': {},
   multiarch/'linux/linux-sigrtminmax.c': {},
   multiarch/'linux/linux-test.c': {},
+  multiarch/'linux/linux-zero-len-iovec.c': {},
   multiarch/'linux/test-vma.c': {},
 }
 
-- 
2.53.0