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

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

On 8/29/24 00:42, Michael Vogt wrote:
> This commit adds support for the `openat2()` syscall in the
> `linux-user` userspace emulator.
> 
> It is implemented by extracting a new helper `maybe_do_fake_open()`
> out of the exiting `do_guest_openat()` and share that with the
> new `do_guest_openat2()`. Unfortunatly we cannot just make
> do_guest_openat2() a superset of do_guest_openat() because the
> openat2() syscall is stricter with the argument checking and
> will return an error for invalid flags or mode combinations (which
> open()/openat() will ignore).
> 
> Note that in this commit using openat2() for a "faked" file in
> /proc will ignore the "resolve" flags. This is not great but it
> seems similar to the exiting behavior when openat() is called
> with a dirfd to "/proc". Here too the fake file lookup may
> not catch the special file because "realpath()" is used to
> determine if the path is in /proc. Alternatively to ignoring
> we could simply fail with `-TARGET_ENOSYS` (or similar) if
> `resolve` flags are passed and we found something that looks
> like a file in /proc that needs faking.
> 
> Signed-off-by: Michael Vogt <mvogt@redhat.com>
> 
> Buglink: https://github.com/osbuild/bootc-image-builder/issues/619
> ---
>   linux-user/qemu.h         |  4 +++
>   linux-user/syscall.c      | 73 ++++++++++++++++++++++++++++++++++++---
>   linux-user/syscall_defs.h |  7 ++++
>   meson.build               |  1 +
>   4 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 2e90a97175..47b6d7da88 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -164,6 +164,10 @@ struct TaskState {
>   abi_long do_brk(abi_ulong new_brk);
>   int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
>                       int flags, mode_t mode, bool safe);
> +#ifdef HAVE_OPENAT2_H
> +int do_guest_openat2(CPUArchState *cpu_env, int dirfd, const char *pathname,
> +                     struct target_open_how *how, bool safe);
> +#endif

Not needed.  The only reason do_guest_openat is declared here is for gdbstub/.

> +#ifdef HAVE_OPENAT2_H
> +#include <linux/openat2.h>
> +/* glibc has no header for SYS_openat2 so we need to get it via syscall.h */
> +#include <sys/syscall.h>
> +#endif

No need for sys/syscall.h, handled by safe_syscall4.

> +#ifdef HAVE_OPENAT2_H
> +int do_guest_openat2(CPUArchState *cpu_env, int dirfd, const char *fname,
> +                     struct target_open_how *how, bool safe)

No need for safe argument, that is only used by gdbstub/.

> +{
> +    /*
> +     * 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.
> +     */
> +    int fd = maybe_do_fake_open(cpu_env, dirfd, fname, how->flags, how->mode,
> +                                safe);
> +    if (fd >= 0)
> +        return fd;
> +
> +    if (safe) {
> +        return safe_openat2(dirfd, fname, (struct open_how *)how,

This cast is an indication that you're doing something wrong.

> +#if defined(TARGET_NR_openat2) && defined(HAVE_OPENAT2_H)
> +    case TARGET_NR_openat2:
> +        {
> +            struct target_open_how how, *target_how;
> +            if (!(p = lock_user_string(arg2)))
> +                return -TARGET_EFAULT;

No assignment within if.

> +            if (!(lock_user_struct(VERIFY_READ, target_how, arg3, 1)))
> +                return -TARGET_EFAULT;
> +            how.flags = target_to_host_bitmask(target_how->flags,
> +                                               fcntl_flags_tbl);
> +            how.mode = tswap64(target_how->mode);
> +            how.resolve = tswap64(target_how->resolve);
> +            ret = get_errno(do_guest_openat2(cpu_env, arg1, p, &how, true));
> +            fd_trans_unregister(ret);
> +            unlock_user_struct(target_how, arg3, 0);
> +            unlock_user(p, arg2, 0);
> +            return ret;
> +        }
> +#endif

Move all of this code into the helper function for clarity.

Missing validation of small arg4 (EINVAL).
Missing validation of zeros for large arg4 (E2BIG).

The 'how' variable should be the host open_how structure.
Then you don't need the incorrect cast above.
Missing a zero of the structure, which in the future might contain extra fields.

Given linux/openat2.h appeared in linux 5.6, we might be coming to the point at which all 
supported host os must have it.  Otherwise, are you using anything besides struct open_how 
itself?  Perhaps we're better off always defining the structure locally, so that way it 
cannot expand on us with a mere upgrade of the system linux headers package?

> +/* from kernel's include/uapi/linux/openat2.h */
> +struct target_open_how {
> +    __u64 flags;
> +    __u64 mode;
> +    __u64 resolve;
> +};

Using host __u64 is always wrong for target structures.
Use "abi_*" types for target alignment requirements.


r~