linux-kernel - Re: [PATCH 2/2] [v2] vduse: fix compat handling for VDUSE_IOTLB_GET_FD/VDUSE_VQ_GET

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: <20260203054152-mutt-send-email-mst@kernel.org>
Date: Tue, 3 Feb 2026 05:47:44 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Arnd Bergmann <arnd@...nel.org>, Jason Wang <jasowang@...hat.com>,
	Xie Yongji <xieyongji@...edance.com>,
	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 Tue, Feb 03, 2026 at 11:39:43AM +0100, Arnd Bergmann wrote:
> On Tue, Feb 3, 2026, at 11:22, Michael S. Tsirkin wrote:
> > 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)
> 
> Sure
> 
> > 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.
> 
> No, the weird alignment rules are only on arc, csky, m68k,
> microblaze, nios2, openrisc, sh and x86-32. Out of those,
> x86 is hte only one that currently has a 64-bit version
> (arc and micrblaze 64-bit support never made it upstream,
> sh64 was removed since there were no products).
> 
> All the other architectures with compat support (arm,
> powerpc, mips, sparc, riscv) have the same alignment rules
> for 32-bit and 64-bit builds and align all integers naturally.


Oh interesting. But the code is compiled for
and generates useless code for all CONFIG_COMPAT right now.

The ifdef you need is COMPAT_FOR_U64_ALIGNMENT then, I think.





> >> + * 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.
> 
> Right, I could remove the padding field here, since this is
> just used to document the size of the otherwise implied
> padding.
> 
> The patch I used to find the issue originally adds explicit
> padding to all uapi structures with implied padding, so I
> did the smae thing here.
> 
> >> +#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?
> 
> My mistake, that should not have been here.
> 
> >> @@ -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.
> 
> Sure. I only used this because you asked for fewer #ifdefs in
> my v1 patch.

It's less the amount of ifdefs more them being placed strategically.
sorry about being unclear.

> If I use an #ifdef around this one, I also have
> to add one around the function definition.

and the structs, preferably.

> In that case, I'd
> probably change it back to the x86 check there, and use
> 
> #if defined(CONFIG_X86_64) && defined(CONFIG_COMPAT)
>       .compat_ioctl = vduse_dev_compat_ioctl,
> #else
>       .compat_ioctl = compat_ptr_ioctl.
> #endif
> 
>       Arnd