| Message ID | 20180912150142.157913-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/17] compat_ioctl: add generic_compat_ioctl_ptrarg() | expand |
On Wed, Sep 12, 2018 at 05:01:03PM +0200, Arnd Bergmann wrote: > Each of these drivers has a copy of the same trivial helper function to > convert the pointer argument and then call the native ioctl handler. > > We now have a generic implementation of that, so use it. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > drivers/char/ppdev.c | 12 +--------- > drivers/char/tpm/tpm_vtpm_proxy.c | 12 +--------- > drivers/firewire/core-cdev.c | 12 +--------- > drivers/hid/usbhid/hiddev.c | 11 +-------- > drivers/hwtracing/stm/core.c | 12 +--------- > drivers/misc/mei/main.c | 22 +---------------- > drivers/mtd/ubi/cdev.c | 36 +++------------------------- > drivers/net/tap.c | 12 +--------- > drivers/staging/pi433/pi433_if.c | 12 +--------- > drivers/usb/core/devio.c | 16 +------------ > drivers/vfio/vfio.c | 39 +++---------------------------- > drivers/vhost/net.c | 12 +--------- > drivers/vhost/scsi.c | 12 +--------- > drivers/vhost/test.c | 12 +--------- > drivers/vhost/vsock.c | 12 +--------- > fs/fat/file.c | 13 +---------- > 16 files changed, 20 insertions(+), 237 deletions(-) > > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c > index 87a0ce47f201..a170f5ca7416 100644 > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c > @@ -678,20 +678,10 @@ static long vtpmx_fops_ioctl(struct file *f, unsigned int ioctl, > } > } > > -#ifdef CONFIG_COMPAT > -static long vtpmx_fops_compat_ioctl(struct file *f, unsigned int ioctl, > - unsigned long arg) > -{ > - return vtpmx_fops_ioctl(f, ioctl, (unsigned long)compat_ptr(arg)); > -} > -#endif > - > static const struct file_operations vtpmx_fops = { > .owner = THIS_MODULE, > .unlocked_ioctl = vtpmx_fops_ioctl, > -#ifdef CONFIG_COMPAT > - .compat_ioctl = vtpmx_fops_compat_ioctl, > -#endif > + .compat_ioctl = generic_compat_ioctl_ptrarg, > .llseek = noop_llseek, > }; For vtpm: Reviewed-by: Jason Gunthorpe <jgg@mellanox.com> Arnd, would you consider including a patch as part of/after this series to make compat_ioctl in drivers/infiniband/core/uverbs_main.c use this as well? Looks like a bug too? Thanks, Jason
From: Arnd Bergmann > Sent: 12 September 2018 16:01 > > The ceph_ioctl function is used both for files and directories, but only > the files support doing that in 32-bit compat mode. > > For consistency, add the same compat handler to the dir operations > as well. Have you verified that all the relevant ioctl buffer structures are exactly the same for 32bit and 64bit applications? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Sep 12, 2018 at 05:01:04PM +0200, Arnd Bergmann wrote: > A handful of drivers all have a trivial wrapper around their ioctl > handler, but don't call the compat_ptr() conversion function at the > moment. In practice this does not matter, since none of them are used > on the s390 architecture and for all other architectures, compat_ptr() > does not do anything, but using the new generic_compat_ioctl_ptrarg > helper makes it more correct in theory, and simplifies the code. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/misc/cxl/flash.c | 8 +------- > drivers/misc/genwqe/card_dev.c | 23 +---------------------- > drivers/scsi/megaraid/megaraid_mm.c | 28 +--------------------------- > drivers/usb/gadget/function/f_fs.c | 12 +----------- > 4 files changed, 4 insertions(+), 67 deletions(-) Nice cleanup on this series! Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Wed, Sep 12, 2018 at 05:01:03PM +0200, Arnd Bergmann wrote: > Each of these drivers has a copy of the same trivial helper function to > convert the pointer argument and then call the native ioctl handler. > > We now have a generic implementation of that, so use it. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Wed, Sep 12, 2018 at 05:01:02PM +0200, Arnd Bergmann wrote: > Many drivers have ioctl() handlers that are completely compatible > between 32-bit and 64-bit architectures, except for the argument > that is passed down from user space and may have to be passed > through compat_ptr() in order to become a valid 64-bit pointer. > > Using ".compat_ptr=generic_compat_ioctl_ptrarg" in file operations > should let us simplify a lot of those drivers to avoid #ifdef > checks, and convert additional drivers that don't have proper > compat handling yet. Just keep in mind that this should *only* be used when all ioctls implemented in a given instance do take pointers. Because otherwise you are asking for trouble - e.g. if one of them takes an u32 used as a bitmap, this will run into trouble as soon as somebody uses bit 31. With no visible warnings. IOW, it shouldn't be used blindly and it should come with big fat warning.
On Thu, Sep 13, 2018 at 4:07 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Wed, Sep 12, 2018 at 05:01:02PM +0200, Arnd Bergmann wrote: > > Many drivers have ioctl() handlers that are completely compatible > > between 32-bit and 64-bit architectures, except for the argument > > that is passed down from user space and may have to be passed > > through compat_ptr() in order to become a valid 64-bit pointer. > > > > Using ".compat_ptr=generic_compat_ioctl_ptrarg" in file operations > > should let us simplify a lot of those drivers to avoid #ifdef > > checks, and convert additional drivers that don't have proper > > compat handling yet. > > Just keep in mind that this should *only* be used when all > ioctls implemented in a given instance do take pointers. > Because otherwise you are asking for trouble - e.g. if one of > them takes an u32 used as a bitmap, this will run into trouble > as soon as somebody uses bit 31. With no visible warnings. > > IOW, it shouldn't be used blindly and it should come with big > fat warning. I was hoping that the _ptrarg suffix gives enough warning here, but maybe not. I was careful to only use it in cases that I checked are safe, either using only pointer arguments, or no arguments. What we might do for further clarification (besides adding a comment next to the declaration), would be to add a complementary generic_compat_ioctl_intarg() that skips the compat_ptr(). There are only a handful of drivers that would use this though. Arnd
On Wed, 12 Sep 2018 17:13:02 +0200, Arnd Bergmann wrote: > > The SNDCTL_* and SOUND_* commands are the old OSS user interface. > > I checked all the sound ioctl commands listed in fs/compat_ioctl.c > to see if we still need the translation handlers. Here is what I > found: > > - sound/oss/ is (almost) gone from the kernel, this is what actually > needed all the translations > - The ALSA emulation for OSS correctly handles all compat_ioctl > commands already. > - sound/oss/dmasound/ is the last holdout of the original OSS code, > this is only used on arch/m68k, which has no 64-bit mode and > hence needs no compat handlers > - arch/um/drivers/hostaudio_kern.c may run in 64-bit mode with > 32-bit x86 user space underneath it. This rare corner case is > the only one that still needs the compat handlers. > > By adding a simple redirect of .compat_ioctl to .unlocked_ioctl in the > UML driver, we can remove all the COMPATIBLE_IOCTL() annotations without > a change in functionality. For completeness, I'm adding the same thing > to the dmasound file, knowing that it makes no difference. > > The compat_ioctl list contains one comment about SNDCTL_DSP_MAPINBUF and > SNDCTL_DSP_MAPOUTBUF, which actually would need a translation handler > if implemented. However, the native implementation just returns -EINVAL, > so we don't care. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Looks good to me. Reviewed-by: Takashi Iwai <tiwai@suse.de> thanks, Takashi
On Wed, Sep 12, 2018 at 05:08:52PM +0200, Arnd Bergmann wrote: > The .ioctl and .compat_ioctl file operations have the same prototype so > they can both point to the same function, which works great almost all > the time when all the commands are compatible. > > One exception is the s390 architecture, where a compat pointer is only > 31 bit wide, and converting it into a 64-bit pointer requires calling > compat_ptr(). Most drivers here will ever run in s390, but since we now > have a generic helper for it, it's easy enough to use it consistently. > > I double-checked all these drivers to ensure that all ioctl arguments > are used as pointers or are ignored, but are not interpreted as integer > values. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- ... > drivers/platform/x86/wmi.c | 2 +- ... > static void link_event_work(struct work_struct *work) > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 04791ea5d97b..e4d0697e07d6 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -886,7 +886,7 @@ static const struct file_operations wmi_fops = { > .read = wmi_char_read, > .open = wmi_char_open, > .unlocked_ioctl = wmi_ioctl, > - .compat_ioctl = wmi_ioctl, > + .compat_ioctl = generic_compat_ioctl_ptrarg, > }; For platform/drivers/x86: Acked-by: Darren Hart (VMware) <dvhart@infradead.org> As for a longer term solution, would it be possible to init fops in such a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg so we don't have to duplicate this boilerplate for every ioctl fops structure? -- Darren Hart VMware Open Source Technology Center
On Mon, Sep 24, 2018 at 10:18:52PM +0200, Arnd Bergmann wrote: > On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Tue, Sep 18, 2018 at 10:51:08AM -0700, Darren Hart wrote: > > > On Fri, Sep 14, 2018 at 09:57:48PM +0100, Al Viro wrote: > > > > On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote: > > > > > > > > > Acked-by: Darren Hart (VMware) <dvhart@infradead.org> > > > > > > > > > > As for a longer term solution, would it be possible to init fops in such > > > > > a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg > > > > > so we don't have to duplicate this boilerplate for every ioctl fops > > > > > structure? > > > > > > > > Bad idea, that... Because several years down the road somebody will add > > > > an ioctl that takes an unsigned int for argument. Without so much as looking > > > > at your magical mystery macro being used to initialize file_operations. > > > > > > Fair, being explicit in the declaration as it is currently may be > > > preferable then. > > > > It would be much cleaner and safer if you could arrange things to add > > something like this to struct file_operations: > > > > long (*ptr_ioctl) (struct file *, unsigned int, void __user *); > > > > Where the core code automatically converts the unsigned long to the > > void __user * as appropriate. > > > > Then it just works right always and the compiler will help address > > Al's concern down the road. > > I think if we wanted to do this with a new file operation, the best > way would be to do the copy_from_user()/copy_to_user() in the caller > as well. > > We already do this inside of some subsystems, notably drivers/media/, > and it simplifies the implementation of the ioctl handler function > significantly. We obviously cannot do this in general, both because of > traditional drivers that have 16-bit command codes (drivers/tty and others) > and also because of drivers that by accident defined the commands > incorrectly and use the wrong type or the wrong direction in the > definition. That could work well, but the first idea could be done globally and mechanically, while this would require very careful per-driver investigation. Particularly if the core code has worse performance.. ie due to kmalloc calls or something. I think it would make more sense to start by having the core do the case to __user and then add another entry point to have the core do the copy_from_user, and so on. Jason
On Thu, Sep 13, 2018 at 12:29:02PM +0200, Arnd Bergmann wrote: > I was hoping that the _ptrarg suffix gives enough warning here, > but maybe not. I was careful to only use it in cases that I > checked are safe, either using only pointer arguments, or > no arguments. > > What we might do for further clarification (besides adding a > comment next to the declaration), would be to add a > complementary generic_compat_ioctl_intarg() that skips > the compat_ptr(). There are only a handful of drivers that > would use this though. ... and the next Monday zeniv went down until the end of September, so I'd missed any resends that might've happened in that window. It's _probably_ too late for this cycle, but let's deal with that thing properly for the next one. A couple of comments from rereading the thread: * generic_compat_ioctl_fthagn^H^H^H^H^H^Hptrarg should not be EXPORT_SYMBOL_GPL(). I'm sorry, but this is beyond ridiculous - "call native ioctl, with the last argument interpreted as an address from 32bit process POV and converted to 64bit equivalent" should not be copyrightable at all, and there's really only one natural way to express that. Use EXPORT_SYMBOL(). And I'd consider names like compat_ptr_ioctl() - easier to type and less opaque... * rtc patch makes RTC_IRQP_SET32 et.al. accepted by 64bit syscall. Which is a behaviour change that might or might not be OK, but it needs to be clearly stated. Could you resend the series, with ACKs attached, etc., either based on -next (if done now) or on -rc1 (once released)?
On Sun, Oct 28, 2018 at 6:07 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Thu, Sep 13, 2018 at 12:29:02PM +0200, Arnd Bergmann wrote: > > > I was hoping that the _ptrarg suffix gives enough warning here, > > but maybe not. I was careful to only use it in cases that I > > checked are safe, either using only pointer arguments, or > > no arguments. > > > > What we might do for further clarification (besides adding a > > comment next to the declaration), would be to add a > > complementary generic_compat_ioctl_intarg() that skips > > the compat_ptr(). There are only a handful of drivers that > > would use this though. > > ... and the next Monday zeniv went down until the end of September, > so I'd missed any resends that might've happened in that window. > > It's _probably_ too late for this cycle, but let's deal with that > thing properly for the next one. Right, I don't think we're in a hurry, so I'll rebase my patches once -rc1 is out and send you the new version. > A couple of comments from rereading the thread: > * generic_compat_ioctl_fthagn^H^H^H^H^H^Hptrarg should not > be EXPORT_SYMBOL_GPL(). I'm sorry, but this is beyond ridiculous - > "call native ioctl, with the last argument interpreted as an address > from 32bit process POV and converted to 64bit equivalent" should > not be copyrightable at all, and there's really only one natural > way to express that. Use EXPORT_SYMBOL(). Sure, I didn't really give this much thought. > And I'd consider names > like compat_ptr_ioctl() - easier to type and less opaque... Good idea. > * rtc patch makes RTC_IRQP_SET32 et.al. accepted by 64bit > syscall. Which is a behaviour change that might or might not be > OK, but it needs to be clearly stated. Yes. I was aware of the change in behavior and have done similar changes for simplicity on y2038 ioctl patches in the past, but you are right that this should be mentioned in the changelog. Arnd
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index a9b00942e87d..2d7c7e149083 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -122,6 +122,16 @@ get_user(val, srcptr) || put_user(val, dstptr); \ }) +/* helper function to avoid trivial compat_ioctl() implementations */ +long generic_compat_ioctl_ptrarg(struct file *file, unsigned int cmd, unsigned long arg) +{ + if (!file->f_op->unlocked_ioctl) + return -ENOIOCTLCMD; + + return file->f_op->unlocked_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); +} +EXPORT_SYMBOL_GPL(generic_compat_ioctl_ptrarg); + static int do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { int err; diff --git a/include/linux/fs.h b/include/linux/fs.h index 33322702c910..18a90aa2dc93 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1643,6 +1643,13 @@ int vfs_mkobj(struct dentry *, umode_t, extern long vfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg); +#ifdef CONFIG_COMPAT +extern long generic_compat_ioctl_ptrarg(struct file *file, unsigned int cmd, + unsigned long arg); +#else +#define generic_compat_ioctl_ptrarg NULL +#endif + /* * VFS file helper functions. */
Many drivers have ioctl() handlers that are completely compatible between 32-bit and 64-bit architectures, except for the argument that is passed down from user space and may have to be passed through compat_ptr() in order to become a valid 64-bit pointer. Using ".compat_ptr=generic_compat_ioctl_ptrarg" in file operations should let us simplify a lot of those drivers to avoid #ifdef checks, and convert additional drivers that don't have proper compat handling yet. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/compat_ioctl.c | 10 ++++++++++ include/linux/fs.h | 7 +++++++ 2 files changed, 17 insertions(+) -- 2.18.0