Re: [PATCH RESEND V12 2/8] fuse: 32-bit user space ioctl compat for fuse device

From: Alessio Balsini <balsini@android.com>
To: Arnd Bergmann <arnd@kernel.org>
Cc: Alessio Balsini <balsini@android.com>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Akilesh Kailash <akailash@google.com>,
	Amir Goldstein <amir73il@gmail.com>,
	Antonio SJ Musumeci <trapexit@spawn.link>,
	David Anderson <dvander@google.com>,
	Giuseppe Scrivano <gscrivan@redhat.com>,
	Jann Horn <jannh@google.com>, Jens Axboe <axboe@kernel.dk>,
	Martijn Coenen <maco@android.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Lawrence <paullawrence@google.com>,
	Peng Tao <bergwolf@gmail.com>,
	Stefano Duo <duostefano93@gmail.com>,
	Zimuzo Ezeozue <zezeozue@google.com>, wuyan <wu-yan@tcl.com>,
	fuse-devel@lists.sourceforge.net,
	Android Kernel Team <kernel-team@android.com>,
	Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RESEND V12 2/8] fuse: 32-bit user space ioctl compat for fuse device
Date: Thu, 18 Mar 2021 16:13:23 +0000	[thread overview]
Message-ID: <YFN8IyFTdqhlS9Lf@google.com> (raw)
In-Reply-To: <CAK8P3a2VDH9-reuj8QTkFzbaU9XTUEOWFCmCVg1Snb6RjD6mHw@mail.gmail.com>

On Tue, Mar 16, 2021 at 07:53:06PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 25, 2021 at 4:48 PM Alessio Balsini <balsini@android.com> wrote:
> >
> > With a 64-bit kernel build the FUSE device cannot handle ioctl requests
> > coming from 32-bit user space.
> > This is due to the ioctl command translation that generates different
> > command identifiers that thus cannot be used for direct comparisons
> > without proper manipulation.
> >
> > Explicitly extract type and number from the ioctl command to enable
> > 32-bit user space compatibility on 64-bit kernel builds.
> >
> > Signed-off-by: Alessio Balsini <balsini@android.com>
> 
> I saw this commit go into the mainline kernel, and I'm worried that this
> doesn't do what the description says. Since the argument is a 'uint32_t',
> it is the same on both 32-bit and 64-bit user space, and the patch won't
> make any difference for compat mode, as long as that is using the normal
> uapi headers.
> 
> If there is any user space that has a different definition of
> FUSE_DEV_IOC_CLONE, that may now successfully call
> this ioctl command, but the kernel will now also accept any other
> command code that has the same type and number, but an
> arbitrary direction or size argument.
> 
> I think this should be changed back to specifically allow the
> command code(s) that are actually used and nothing else.
> 
>        Arnd

Hi Arnd,

Thanks for spotting this possible criticality.

I noticed that 32-bit users pace was unable to use the
FUSE_DEV_IOC_CLONE ioctl on 64-bit kernels, so this change avoid this
issue by forcing the kernel to interpret 32 and 64 bit
FUSE_DEV_IOC_CLONE command as if they were the same.
This is the simplest solution I could find as the UAPI is not changed
as, as you mentioned, the argument doesn't require any conversion.

I understand that this might limit possible future extensions of the
FUSE_DEV_IOC_XXX ioctls if their in/out argument changed depending on
the architecture, but only at that point we can switch to using the
compat layer, right?

What I'm worried about is the direction, do you think this would be an
issue?

I can start working on a compat layer fix meanwhile.

Thanks,
Alessio