Re: [PATCH v2 1/1] linux-user: add openat2 support in linux-user

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

On 8/30/24 00:44, Michael Vogt wrote:
> +static int maybe_do_fake_open(CPUArchState *cpu_env, int dirfd,
> +                              const char *fname, int flags, mode_t mode,
> +                              bool safe, bool *use_returned_fd)
>   {
>       g_autofree char *proc_name = NULL;
>       const char *pathname;
> @@ -8362,6 +8371,7 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>   #endif
>           { NULL, NULL, NULL }
>       };
> +    *use_returned_fd = true;
>   
>       /* if this is a file from /proc/ filesystem, expand full name */
>       proc_name = realpath(fname, NULL);
> @@ -8418,12 +8428,87 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>           return fd;
>       }
>   
> +    *use_returned_fd = false;
> +    return -1;
> +}

Why is -1 insufficient for signalling "do not use"?


> +
> +int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
> +                    int flags, mode_t mode, bool safe)
> +{
> +    bool use_returned_fd;
> +    int fd = maybe_do_fake_open(cpu_env, dirfd, fname, flags, mode, safe,
> +                                &use_returned_fd);
> +    if (use_returned_fd)
> +        return fd;

Braces are required.  Use scripts/checkpatch.pl.

> +#ifdef HAVE_OPENAT2_H
> +static int do_guest_openat2(CPUArchState *cpu_env, int dirfd, const char *fname,
> +                            struct open_how *how)
> +{
> +    /*
> +     * Ideally we would pass "how->resolve" flags into this helper too but
> +     * the lookup for files that need faking is based on "realpath()" so
> +     * neither a dirfd for "proc" nor restrictions via "resolve" flags can
> +     * be honored right now.
> +     */
> +    bool use_returned_fd;
> +    int fd = maybe_do_fake_open(cpu_env, dirfd, fname, how->flags, how->mode,
> +                                true, &use_returned_fd);
> +    if (use_returned_fd)
> +        return fd;
> +
> +    return safe_openat2(dirfd, fname, how, sizeof(struct open_how));
> +}

I don't think this needs to be a separate function.
We did that for do_guest_openat for gdbstub.

> +
> +static int do_openat2(CPUArchState *cpu_env, abi_long arg1, abi_long arg2,
> +                      abi_long arg3, abi_long arg4)

You might as well name the arguments properly, and use abi_ptr/abi_ulong where it makes sense.

> +{
> +    struct open_how how = {0};
> +    struct target_open_how *target_how = NULL;
> +    int ret;
> +
> +    char *p = lock_user_string(arg2);
> +    if (!p) {
> +        ret = -TARGET_EFAULT;
> +        goto out;
> +    }
> +    if (!(lock_user_struct(VERIFY_READ, target_how, arg3, 1))) {
> +        ret = -TARGET_EFAULT;
> +        goto out;
> +    }
> +    size_t target_open_how_struct_size = arg4;
> +    if (target_open_how_struct_size < sizeof(struct target_open_how)) {
> +        ret = -TARGET_EINVAL;
> +        goto out;
> +    }

These checks should be in the same order as the kernel:

SYSCALL_DEFINE(openat2)
     usize < HOW_SIZE_VER0 -> EINVAL
     copy_struct_from_user(how) -> E2BIG

all come before examining the path argument.

> +    if (target_open_how_struct_size > sizeof(struct target_open_how)) {
> +        qemu_log_mask(LOG_UNIMP, "Unimplemented openat2 open_how size: %lu\n",
> +                      target_open_how_struct_size);
> +        ret = -TARGET_E2BIG;
> +        goto out;
>       }

 From copy_struct_from_user you're missing

         } else if (usize > ksize) {
                 int ret = check_zeroed_user(src + size, rest);
                 if (ret <= 0)
                         return ret ?: -E2BIG;

It's not just testing the size, it's reading the following bytes and checking for zeros.

It would be worth adding a helper function for this, matching the kernel.  I'm sure there 
are other places in linux-user that should be doing the same thing.


> +    how.flags = target_to_host_bitmask(target_how->flags, fcntl_flags_tbl);
> +    how.mode = tswap64(target_how->mode);
> +    how.resolve = tswap64(target_how->resolve);

With a linux-user copy_struct_from_user, I expect you'd swap in place:

     how.mode = tswap64(how.mode);

etc.


r~