| Message ID | 20200908213715.3553098-1-arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] scsi: aacraid: improve compat_ioctl handlers | expand |
On Tue, Sep 08, 2020 at 11:36:21PM +0200, Arnd Bergmann wrote: > @@ -243,8 +244,23 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg) > struct list_head * entry; > unsigned long flags; > > - if(copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl))) > - return -EFAULT; > + if (in_compat_syscall()) { > + struct compat_fib_ioctl { > + u32 fibctx; > + s32 wait; > + compat_uptr_t fib; > + } cf; I find the struct declaration deep down in the function a little annoying. But otherwise this looks good; Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Sep 08, 2020 at 11:36:22PM +0200, Arnd Bergmann wrote: > It sounds unwise to let user space pass an unchecked 32-bit > offset into a kernel structure in an ioctl. This is an unsigned > variable, so checking the upper bound for the size of the structure > it points into is sufficient to avoid data corruption, but as > the pointer might also be unaligned, it has to be written carefully > as well. > > While I stumbled over this problem by reading the code, I did not > continue checking the function for further problems like it. Oh, yikes! > > Cc: stable@vger.kernel.org What about a Fixes tag instead? > if (ioc->sense_len) { > + /* make sure the pointer is part of the frame */ > + if (ioc->sense_off > (sizeof(union megasas_frame) - sizeof(__le64))) { No need for the inner braces and please avoid over 80 char lines. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Sat, Sep 12, 2020 at 9:20 AM Christoph Hellwig <hch@infradead.org> wrote: > On Tue, Sep 08, 2020 at 11:36:22PM +0200, Arnd Bergmann wrote: > > Cc: stable@vger.kernel.org > > What about a Fixes tag instead? Sure, I can add that. It's been broken since 2.6.15 though, when the driver was initially merged. Arnd
On Sat, Sep 12, 2020 at 9:09 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Sep 08, 2020 at 11:36:21PM +0200, Arnd Bergmann wrote: > > @@ -243,8 +244,23 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg) > > struct list_head * entry; > > unsigned long flags; > > > > - if(copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl))) > > - return -EFAULT; > > + if (in_compat_syscall()) { > > + struct compat_fib_ioctl { > > + u32 fibctx; > > + s32 wait; > > + compat_uptr_t fib; > > + } cf; > > I find the struct declaration deep down in the function a little > annoying. > > But otherwise this looks good; > > Reviewed-by: Christoph Hellwig <hch@lst.de> I got back to these patches now and moved the struct definition, plus fixed a typo I noticed while doing so. Thanks! Arnd
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c index 59e82a832042..6d6f72d68164 100644 --- a/drivers/scsi/aacraid/commctrl.c +++ b/drivers/scsi/aacraid/commctrl.c @@ -25,6 +25,7 @@ #include <linux/completion.h> #include <linux/dma-mapping.h> #include <linux/blkdev.h> +#include <linux/compat.h> #include <linux/delay.h> /* ssleep prototype */ #include <linux/kthread.h> #include <linux/uaccess.h> @@ -243,8 +244,23 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg) struct list_head * entry; unsigned long flags; - if(copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl))) - return -EFAULT; + if (in_compat_syscall()) { + struct compat_fib_ioctl { + u32 fibctx; + s32 wait; + compat_uptr_t fib; + } cf; + + if (copy_from_user(&f, arg, sizeof(struct compat_fib_ioctl))) + return -EFAULT; + + f.fibctx = cf.fibctx; + f.wait = cf.wait; + f.fib = compat_ptr(cf.fib); + } else { + if (copy_from_user(&f, arg, sizeof(struct fib_ioctl))) + return -EFAULT; + } /* * Verify that the HANDLE passed in was a valid AdapterFibContext * diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c index 8588da0a0655..8c0f55845138 100644 --- a/drivers/scsi/aacraid/linit.c +++ b/drivers/scsi/aacraid/linit.c @@ -1182,63 +1182,6 @@ static long aac_cfg_ioctl(struct file *file, return aac_do_ioctl(aac, cmd, (void __user *)arg); } -#ifdef CONFIG_COMPAT -static long aac_compat_do_ioctl(struct aac_dev *dev, unsigned cmd, unsigned long arg) -{ - long ret; - switch (cmd) { - case FSACTL_MINIPORT_REV_CHECK: - case FSACTL_SENDFIB: - case FSACTL_OPEN_GET_ADAPTER_FIB: - case FSACTL_CLOSE_GET_ADAPTER_FIB: - case FSACTL_SEND_RAW_SRB: - case FSACTL_GET_PCI_INFO: - case FSACTL_QUERY_DISK: - case FSACTL_DELETE_DISK: - case FSACTL_FORCE_DELETE_DISK: - case FSACTL_GET_CONTAINERS: - case FSACTL_SEND_LARGE_FIB: - ret = aac_do_ioctl(dev, cmd, (void __user *)arg); - break; - - case FSACTL_GET_NEXT_ADAPTER_FIB: { - struct fib_ioctl __user *f; - - f = compat_alloc_user_space(sizeof(*f)); - ret = 0; - if (clear_user(f, sizeof(*f))) - ret = -EFAULT; - if (copy_in_user(f, (void __user *)arg, sizeof(struct fib_ioctl) - sizeof(u32))) - ret = -EFAULT; - if (!ret) - ret = aac_do_ioctl(dev, cmd, f); - break; - } - - default: - ret = -ENOIOCTLCMD; - break; - } - return ret; -} - -static int aac_compat_ioctl(struct scsi_device *sdev, unsigned int cmd, - void __user *arg) -{ - struct aac_dev *dev = (struct aac_dev *)sdev->host->hostdata; - if (!capable(CAP_SYS_RAWIO)) - return -EPERM; - return aac_compat_do_ioctl(dev, cmd, (unsigned long)arg); -} - -static long aac_compat_cfg_ioctl(struct file *file, unsigned cmd, unsigned long arg) -{ - if (!capable(CAP_SYS_RAWIO)) - return -EPERM; - return aac_compat_do_ioctl(file->private_data, cmd, arg); -} -#endif - static ssize_t aac_show_model(struct device *device, struct device_attribute *attr, char *buf) { @@ -1523,7 +1466,7 @@ static const struct file_operations aac_cfg_fops = { .owner = THIS_MODULE, .unlocked_ioctl = aac_cfg_ioctl, #ifdef CONFIG_COMPAT - .compat_ioctl = aac_compat_cfg_ioctl, + .compat_ioctl = aac_cfg_ioctl, #endif .open = aac_cfg_open, .llseek = noop_llseek, @@ -1536,7 +1479,7 @@ static struct scsi_host_template aac_driver_template = { .info = aac_info, .ioctl = aac_ioctl, #ifdef CONFIG_COMPAT - .compat_ioctl = aac_compat_ioctl, + .compat_ioctl = aac_ioctl, #endif .queuecommand = aac_queuecommand, .bios_param = aac_biosparm,
The use of compat_alloc_user_space() can be easily replaced by handling compat arguments in the regular handler, and this will make it work for big-endian kernels as well, which at the moment get an invalid indirect pointer argument. Calling aac_ioctl() instead of aac_compat_do_ioctl() means the compat and native code paths behave the same way again, which they stopped when the adapter health check was added only in the native function. Fixes: 572ee53a9bad ("scsi: aacraid: check adapter health") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/scsi/aacraid/commctrl.c | 20 +++++++++-- drivers/scsi/aacraid/linit.c | 61 ++------------------------------- 2 files changed, 20 insertions(+), 61 deletions(-) -- 2.27.0