Re: [PATCH 14/22] bsd-user/freebsd/os-syscall.c: lock_iovec

[Kyle Evans <kevans@freebsd.org>]

On Tue, Feb 1, 2022 at 5:15 AM Warner Losh <imp@bsdimp.com> wrote:
>
> lock_iovec will lock an I/O vec and the memory to which it referrs and
> create a iovec in the host space that referrs to it, with full error
> unwinding.
>

s/referrs/refers/ twice

> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>  bsd-user/freebsd/os-syscall.c | 92 +++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
>

Two typos, otherwise seems to LGTM:

Reviewed-by: Kyle Evans <kevans@FreeBSD.org>

> diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
> index 060134a9ecd..c21759ae7ce 100644
> --- a/bsd-user/freebsd/os-syscall.c
> +++ b/bsd-user/freebsd/os-syscall.c
> @@ -75,6 +75,98 @@ bool is_error(abi_long ret)
>      return (abi_ulong)ret >= (abi_ulong)(-4096);
>  }
>
> +struct iovec *lock_iovec(int type, abi_ulong target_addr,
> +        int count, int copy)
> +{
> +    struct target_iovec *target_vec;
> +    struct iovec *vec;
> +    abi_ulong total_len, max_len;
> +    int i;
> +    int err = 0;
> +    bool bad_address = false;
> +
> +    if (count == 0) {
> +        errno = 0;
> +        return NULL;
> +    }
> +    if (count < 0 || count > IOV_MAX) {
> +        errno = EINVAL;
> +        return NULL;
> +    }
> +
> +    vec = calloc(count, sizeof(struct iovec));
> +    if (vec == NULL) {
> +        errno = ENOMEM;
> +        return NULL;
> +    }
> +
> +    target_vec = lock_user(VERIFY_READ, target_addr,
> +                           count * sizeof(struct target_iovec), 1);
> +    if (target_vec == NULL) {
> +        err = EFAULT;
> +        goto fail2;
> +    }
> +
> +    /*
> +     * ??? If host page size > target page size, this will result in a value
> +     * larger than what we can actually support.
> +     */
> +    max_len = 0x7fffffff & TARGET_PAGE_MASK;
> +    total_len = 0;
> +
> +    for (i = 0; i < count; i++) {
> +        abi_ulong base = tswapal(target_vec[i].iov_base);
> +        abi_long len = tswapal(target_vec[i].iov_len);
> +
> +        if (len < 0) {
> +            err = EINVAL;
> +            goto fail;
> +        } else if (len == 0) {
> +            /* Zero length pointer is ignored.  */
> +            vec[i].iov_base = 0;
> +        } else {
> +            vec[i].iov_base = lock_user(type, base, len, copy);
> +            /*
> +             * If the first buffer pointer is bad, this is a fault.  But
> +             * subsequent bad buffers will result in a partial write; this is
> +             * realized by filling the vector with null pointers and zero
> +             * lengths.
> +             */
> +            if (!vec[i].iov_base) {
> +                if (i == 0) {
> +                    err = EFAULT;
> +                    goto fail;
> +                } else {
> +                    bad_address = true;
> +                }
> +            }
> +            if (bad_address) {
> +                len = 0;
> +            }
> +            if (len > max_len - total_len) {
> +                len = max_len - total_len;
> +            }
> +        }
> +        vec[i].iov_len = len;
> +        total_len += len;
> +    }
> +
> +    unlock_user(target_vec, target_addr, 0);
> +    return vec;
> +
> + fail:
> +    while (--i >= 0) {
> +        if (tswapal(target_vec[i].iov_len) > 0) {
> +            unlock_user(vec[i].iov_base, tswapal(target_vec[i].iov_base), 0);
> +        }
> +    }
> +    unlock_user(target_vec, target_addr, 0);
> + fail2:
> +    free(vec);
> +    errno = err;
> +    return NULL;
> +}
> +
>  /*
>   * do_syscall() should always have a single exit point at the end so that
>   * actions, such as logging of syscall results, can be performed.  All errnos
> --
> 2.33.1
>