Re: [PATCH v2] linux-user: Add most IFTUN ioctls

[Laurent Vivier <laurent@vivier.eu>]

Le 24/07/2020 à 01:10, Shu-Chun Weng a écrit :
> The three options handling `struct sock_fprog` (TUNATTACHFILTER,
> TUNDETACHFILTER, and TUNGETFILTER) are not implemented. Linux kernel
> keeps a user space pointer in them which we cannot correctly handle.
> 
> Signed-off-by: Josh Kunz <jkz@google.com>
> Signed-off-by: Shu-Chun Weng <scw@google.com>
> ---
> v2:
>   Title changed from "linux-user: Add several IFTUN ioctls"
> 
>   Properly specify the argument types for various options, including a custom
>   implementation for TUNSETTXFILTER.
> 
>   #ifdef guards for macros introduced up to 5 years ago.
> 
>  linux-user/ioctls.h       | 45 +++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall.c      | 36 +++++++++++++++++++++++++++++++
>  linux-user/syscall_defs.h | 32 ++++++++++++++++++++++++++++
>  3 files changed, 113 insertions(+)
> 
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index 0713ae1311..b9fb01f558 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -593,3 +593,48 @@
>    IOCTL(KCOV_DISABLE, 0, TYPE_NULL)
>    IOCTL(KCOV_INIT_TRACE, IOC_R, TYPE_ULONG)
>  #endif
> +
> +  IOCTL(TUNSETDEBUG,     IOC_W, TYPE_INT)
> +  IOCTL(TUNSETIFF,       IOC_RW, MK_PTR(MK_STRUCT(STRUCT_short_ifreq)))

Why is this IOC_RW?

> +  IOCTL(TUNSETPERSIST,   IOC_W, TYPE_INT)
> +  IOCTL(TUNSETOWNER,     IOC_W, TYPE_INT)
> +  IOCTL(TUNSETLINK,      IOC_W, TYPE_INT)
> +  IOCTL(TUNSETGROUP,     IOC_W, TYPE_INT)
> +  IOCTL(TUNGETFEATURES,  IOC_R, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETOFFLOAD,   IOC_W, TYPE_LONG)

Why is this long?

> +  IOCTL_SPECIAL(TUNSETTXFILTER, IOC_W, do_ioctl_TUNSETTXFILTER,
> +                /*
> +                 * We can't represent `struct tun_filter` in thunk so leaving
> +                 * this empty. do_ioctl_TUNSETTXFILTER will do the conversion.
> +                 */
> +                TYPE_NULL)

You should use TYPE_PTRVOID to allow QEMU_STRACE to display the pointer.
Or implement the function to dump the structure (see STRUCT_termios and
struct_termios_def).

> +  IOCTL(TUNGETIFF,       IOC_R, MK_PTR(MK_STRUCT(STRUCT_short_ifreq)))
> +  IOCTL(TUNGETSNDBUF,    IOC_R, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETSNDBUF,    IOC_W, MK_PTR(TYPE_INT))
> +  /*
> +   * TUNATTACHFILTER and TUNDETACHFILTER are not supported. Linux kernel keeps a
> +   * user pointer in TUNATTACHFILTER, which we are not able to correctly handle.
> +   */
> +  IOCTL(TUNGETVNETHDRSZ, IOC_R, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETVNETHDRSZ, IOC_W, MK_PTR(TYPE_INT))
> +  IOCTL(TUNSETQUEUE,     IOC_W, MK_PTR(MK_STRUCT(STRUCT_short_ifreq)))
> +  IOCTL(TUNSETIFINDEX ,  IOC_W, MK_PTR(TYPE_INT))
> +  /* TUNGETFILTER is not supported: see TUNATTACHFILTER. */
> +  IOCTL(TUNSETVNETLE,    IOC_W, MK_PTR(TYPE_INT))
> +  IOCTL(TUNGETVNETLE,    IOC_R, MK_PTR(TYPE_INT))
> +#ifdef TUNSETVNETBE
> +  IOCTL(TUNSETVNETBE,    IOC_W, MK_PTR(TYPE_INT))
> +  IOCTL(TUNGETVNETBE,    IOC_R, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNSETSTEERINGEBPF
> +  IOCTL(TUNSETSTEERINGEBPF, IOC_W, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNSETFILTEREBPF
> +  IOCTL(TUNSETFILTEREBPF, IOC_W, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNSETCARRIER
> +  IOCTL(TUNSETCARRIER,   IOC_W, MK_PTR(TYPE_INT))
> +#endif
> +#ifdef TUNGETDEVNETNS
> +  IOCTL(TUNGETDEVNETNS,  IOC_R, TYPE_NULL)
> +#endif
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1211e759c2..7f1efed189 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -56,6 +56,7 @@
>  #include <linux/wireless.h>
>  #include <linux/icmp.h>
>  #include <linux/icmpv6.h>
> +#include <linux/if_tun.h>
>  #include <linux/errqueue.h>
>  #include <linux/random.h>
>  #ifdef CONFIG_TIMERFD
> @@ -5422,6 +5423,41 @@ static abi_long do_ioctl_drm(const IOCTLEntry *ie, uint8_t *buf_temp,
>  
>  #endif
>  
> +static abi_long do_ioctl_TUNSETTXFILTER(const IOCTLEntry *ie, uint8_t *buf_temp,
> +                                        int fd, int cmd, abi_long arg)
> +{
> +    struct tun_filter *filter = (struct tun_filter *)buf_temp;
> +    struct tun_filter *target_filter;
> +    char *target_addr;
> +
> +    assert(ie->access == IOC_W);
> +
> +    target_filter = lock_user(VERIFY_READ, arg, sizeof(*filter), 1);

sizeof(*target_filter) vould be more coherent: we lock the target memory
so use the size of the type of the target.

> +    if (!target_filter) {
> +        return -TARGET_EFAULT;
> +    }
> +    filter->flags = tswap16(target_filter->flags);
> +    filter->count = tswap16(target_filter->count);
> +    unlock_user(target_filter, arg, sizeof(*filter));

unlock_user(target_filter, arg, 0) as we don't need to copy the value
back to the target.

> +
> +    if (filter->count) {
> +        if (sizeof(*filter) + filter->count * ETH_ALEN > MAX_STRUCT_SIZE) {

Rather than sizeof() use offsetof(struct tun_filter, addr)

> +            return -TARGET_EFAULT;
> +        }
> +
> +        target_addr = lock_user(VERIFY_READ, arg + sizeof(*filter),

Rather than sizeof() use offsetof(struct tun_filter, addr)

> +                                filter->count * ETH_ALEN, 1);
> +        if (!target_addr) {
> +            return -TARGET_EFAULT;
> +        }
> +        memcpy(filter->addr, target_addr, filter->count * ETH_ALEN);
> +        unlock_user(target_addr, arg + sizeof(*filter),

 offsetof(struct tun_filter, addr)

Thanks,
Laurent