| 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
| ||
|
Message-ID: <20260203050216-mutt-send-email-mst@kernel.org>
Date: Tue, 3 Feb 2026 05:22:35 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Arnd Bergmann <arnd@...nel.org>
Cc: Jason Wang <jasowang@...hat.com>, Xie Yongji <xieyongji@...edance.com>,
Arnd Bergmann <arnd@...db.de>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Eugenio Pérez <eperezma@...hat.com>,
virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] [v2] vduse: fix compat handling for
VDUSE_IOTLB_GET_FD/VDUSE_VQ_GET_INFO
On Mon, Feb 02, 2026 at 11:48:08PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
>
> These two ioctls are incompatible on 32-bit x86 userspace, because
> the data structures are shorter than they are on 64-bit.
>
> Add a proper .compat_ioctl handler for x86 that reads the structures
> with the smaller padding before calling the internal handlers.
>
> Fixes: ad146355bfad ("vduse: Support querying information of IOVA regions")
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
> The code is directly copied from the native ioctl handler, but I
> did not test this with actual x86-32 userspace, so please review
> carefully.
More importantly, I'm not applying this until it's tested)
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 123 ++++++++++++++++++++++++++++-
> 1 file changed, 122 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 405d59610f76..3ada167ac260 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1618,6 +1618,127 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> return ret;
> }
>
> +/*
ifndef CONFIG_COMPAT around the structs will make it clearer
they are only for this purpose.
> + * i386 has different alignment constraints than x86_64,
why i386 specifically? many architectures have CONFIG_COMPAT
and it looks like all of them will have the issue.
> + * so there are only 3 bytes of padding instead of 7.
> + */
> +struct compat_vduse_iotlb_entry {
> + compat_u64 offset;
> + compat_u64 start;
> + compat_u64 last;
> + __u8 perm;
> + __u8 padding[__alignof__(compat_u64) - 1];
Was surprised to learn __alignof__ can be used to size
arrays. This is the first use of this capability in the kernel.
I think the point of all this is that compat_vduse_iotlb_entry
will be 4 byte aligned now? Very well. But why do we bother
with specifying the hidden padding? compilers adds exactly
this amount anyway. Just plan compat_u64 will do the trick.
> +};
> +#define COMPAT_VDUSE_IOTLB_GET_FD _IOWR(VDUSE_BASE, 0x10, struct compat_vduse_iotlb_entry)
> +
> +struct compat_vduse_vq_info {
> + __u32 index;
> + __u32 num;
> + compat_u64 desc_addr;
> + compat_u64 driver_addr;
> + compat_u64 device_addr;
> + union {
> + struct vduse_vq_state_split split;
> + struct vduse_vq_state_packed packed;
> + };
> + __u8 ready;
> + __u8 padding[__alignof__(compat_u64) - 1];
> +} __uapi_arch_align;
it's a global variable? I'm not aware of this trick. What is this doing?
> +#define COMPAT_VDUSE_VQ_GET_INFO _IOWR(VDUSE_BASE, 0x15, struct compat_vduse_vq_info)
> +
> +static long vduse_dev_compat_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct vduse_dev *dev = file->private_data;
> + void __user *argp = (void __user *)arg;
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_X86_64))
> + return vduse_dev_ioctl(file, cmd,
> + (unsigned long)compat_ptr(arg));
> +
> + if (unlikely(dev->broken))
> + return -EPERM;
> +
> + switch (cmd) {
> + case COMPAT_VDUSE_IOTLB_GET_FD: {
> + struct vduse_iotlb_entry_v2 entry = {0};
> + struct file *f = NULL;
> +
> + ret = -EFAULT;
> + if (copy_from_user(&entry, argp, _IOC_SIZE(cmd)))
> + break;
> +
> + ret = vduse_dev_iotlb_entry(dev, &entry, &f, NULL);
> + if (ret)
> + break;
> +
> + ret = -EINVAL;
> + if (!f)
> + break;
> +
> + ret = copy_to_user(argp, &entry, _IOC_SIZE(cmd));
> + if (ret) {
> + ret = -EFAULT;
> + fput(f);
> + break;
> + }
> + ret = receive_fd(f, NULL, perm_to_file_flags(entry.perm));
> + fput(f);
> + break;
> + }
> + case COMPAT_VDUSE_VQ_GET_INFO: {
> + struct vduse_vq_info vq_info = {};
> + struct vduse_virtqueue *vq;
> + u32 index;
> +
> + ret = -EFAULT;
> + if (copy_from_user(&vq_info, argp,
> + sizeof(struct compat_vduse_vq_info)))
> + break;
> +
> + ret = -EINVAL;
> + if (vq_info.index >= dev->vq_num)
> + break;
> +
> + index = array_index_nospec(vq_info.index, dev->vq_num);
> + vq = dev->vqs[index];
> + vq_info.desc_addr = vq->desc_addr;
> + vq_info.driver_addr = vq->driver_addr;
> + vq_info.device_addr = vq->device_addr;
> + vq_info.num = vq->num;
> +
> + if (dev->driver_features & BIT_ULL(VIRTIO_F_RING_PACKED)) {
> + vq_info.packed.last_avail_counter =
> + vq->state.packed.last_avail_counter;
> + vq_info.packed.last_avail_idx =
> + vq->state.packed.last_avail_idx;
> + vq_info.packed.last_used_counter =
> + vq->state.packed.last_used_counter;
> + vq_info.packed.last_used_idx =
> + vq->state.packed.last_used_idx;
> + } else
> + vq_info.split.avail_index =
> + vq->state.split.avail_index;
> +
> + vq_info.ready = vq->ready;
> +
> + ret = -EFAULT;
> + if (copy_to_user(argp, &vq_info,
> + sizeof(struct compat_vduse_vq_info)))
> + break;
> +
> + ret = 0;
> + break;
> + }
> + default:
> + ret = -ENOIOCTLCMD;
> + break;
> + }
> +
> + return vduse_dev_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> +}
> +
> static int vduse_dev_release(struct inode *inode, struct file *file)
> {
> struct vduse_dev *dev = file->private_data;
> @@ -1678,7 +1799,7 @@ static const struct file_operations vduse_dev_fops = {
> .write_iter = vduse_dev_write_iter,
> .poll = vduse_dev_poll,
> .unlocked_ioctl = vduse_dev_ioctl,
> - .compat_ioctl = compat_ptr_ioctl,
> + .compat_ioctl = PTR_IF(IS_ENABLED(CONFIG_COMPAT), vduse_dev_compat_ioctl),
Too funky IMHO. Everyone uses ifdef around this, let's do the same.
> .llseek = noop_llseek,
> };
>
> --
> 2.39.5
Powered by blists - more mailing lists