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

["Alex Bennée" <alex.bennee@linaro.org>]

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