linux-kernel - Re: [Stratos-dev] [PATCH 2/2] xen: privcmd: Add support for ioeventfd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <268a2031-63b8-4c7d-b1e5-8ab83ca80b4a@app.fastmail.com>
Date:   Sat, 14 Oct 2023 14:19:52 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Viresh Kumar" <viresh.kumar@...aro.org>,
        "Juergen Gross" <jgross@...e.com>,
        "Stefano Stabellini" <sstabellini@...nel.org>,
        "Oleksandr Tyshchenko" <oleksandr_tyshchenko@...m.com>
Cc:     "Stratos Mailing List" <stratos-dev@...lists.linaro.org>,
        "Erik Schilling" <erik.schilling@...aro.org>,
        "Manos Pitsidianakis" <manos.pitsidianakis@...aro.org>,
        linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org
Subject: Re: [Stratos-dev] [PATCH 2/2] xen: privcmd: Add support for ioeventfd

On Tue, Aug 29, 2023, at 14:29, Viresh Kumar via Stratos-dev wrote:

> +/* For privcmd_ioeventfd::flags */
> +#define PRIVCMD_IOEVENTFD_FLAG_DEASSIGN (1 << 0)
> +
> +struct privcmd_ioeventfd {
> +	void __user *ioreq;
> +	unsigned int __user *ports;
> +	__u64 addr;
> +	__u32 addr_len;
> +	__u32 event_fd;
> +	__u32 vcpus;
> +	__u32 vq;
> +	__u32 flags;
> +	domid_t dom;
> +	__u8 pad[2];
> +};

Using indirect pointers in an ioctl command argument means that
the layout is architecture specific, in particular you can't
use the same one from 32-bit compat tasks. The general recommendation
is to have __u64 members and use u64_to_user_ptr() to access it
from the kernel if you are unable to avoid the pointers altogether.

>  /*
>   * @cmd: IOCTL_PRIVCMD_HYPERCALL
>   * @arg: &privcmd_hypercall_t
> @@ -139,5 +155,7 @@ struct privcmd_irqfd {
>  	_IOC(_IOC_NONE, 'P', 7, sizeof(struct privcmd_mmap_resource))
>  #define IOCTL_PRIVCMD_IRQFD					\
>  	_IOC(_IOC_NONE, 'P', 8, sizeof(struct privcmd_irqfd))
> +#define IOCTL_PRIVCMD_IOEVENTFD					\
> +	_IOC(_IOC_NONE, 'P', 9, sizeof(struct privcmd_ioeventfd))

_IOC() an internal helper that you should not use in driver code.
In particular, you go the data direction wrong here, which breaks
a number of tools, as having "_IOC_NONE" should never be paired with
a nonzero size.

Instead, you should use the existing _IOWR() or _IOR() macros without
the 'sizeof()' like

#define IOCTL_PRIVCMD_IOEVENTFD _IOWR('P', 9, struct privcmd_ioeventfd)

You clearly copied this from the other ioctl command definitions in
the same file that we can't easily fix without breaking the user
ABI, but I still think you should try to do it correctly for new
commands.

     Arnd