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

[Richard Henderson <richard.henderson@linaro.org>]

On 2/1/22 22:14, Warner Losh 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.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   bsd-user/freebsd/os-syscall.c | 92 +++++++++++++++++++++++++++++++++++
>   1 file changed, 92 insertions(+)
> 
> 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));

g_try_new0.

You may want to use g_autofree to simplify error handling, which then requires you use

     return g_steal_pointer(&vec);

on the success path.

> +    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;
> +            }

Surely this bad_address check should happen earlier, before we attempt the lock above?
E.g.

     else if (len == 0 || bad_address)


r~