| Message ID | 20180912150142.157913-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/17] compat_ioctl: add generic_compat_ioctl_ptrarg() | expand |
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 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(+)