| Message ID | 20200908213715.3553098-3-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] scsi: aacraid: improve compat_ioctl handlers | expand |
On Tue, Sep 08, 2020 at 11:36:23PM +0200, Arnd Bergmann wrote: > There have been several attempts to fix serious problems > in the compat handling in megasas_mgmt_compat_ioctl_fw(), > and it also uses the compat_alloc_user_space() function. I just looked into this a few weeks ago but didn't get that far.. > +static struct megasas_iocpacket *megasas_compat_iocpacket_get_user(void __user *arg) Pointlessly long line. > +{ > + int err = -EFAULT; > +#ifdef CONFIG_COMPAT I find the ifdef inside the function a little weird. Doing it in the caller would be a little less bad. What I ended up doing in my unfinished patch was to move the compat handling into a new megaraid_sas_compat.c file, so we'd always get the prototypes in a header, but given that all the calls are eliminated for the !COMPAT case we'd avoid ifdefs entirely, but having that file for a single function is also rather silly. > + struct megasas_iocpacket *ioc; > + struct compat_megasas_iocpacket __user *cioc = arg; > + int i; > + > + ioc = kzalloc(sizeof(*ioc), GFP_KERNEL); Missing NULL check here. > + if (copy_from_user(ioc, arg, > + offsetof(struct megasas_iocpacket, frame) + 128)) > + goto out; the 128 here while copied from the original code should probably be replaced with a sizeof(frame->raw). > + if (ioc->sense_len) { > + compat_uptr_t *sense_ioc_ptr; > + void __user *sense_cioc; > + > + /* make sure the pointer is inside of frame.raw */ > + if (ioc->sense_off > > + (sizeof(ioc->frame.raw) - sizeof(void __user*))) { > + err = -EINVAL; > + goto out; > + } > + > + sense_ioc_ptr = (compat_uptr_t *)&ioc->frame.raw[ioc->sense_off]; > + sense_cioc = compat_ptr(get_unaligned(sense_ioc_ptr)); > + put_unaligned((unsigned long)sense_cioc, (void **)sense_ioc_ptr); I think we should really handle this where the sense point is set up. This is the untested hunk I had: diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c index 48fad675b5ed02..c3ddcfce86df50 100644 --- a/drivers/scsi/megaraid/megaraid_sas_base.c +++ b/drivers/scsi/megaraid/megaraid_sas_base.c @@ -8231,7 +8231,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance, goto out; } - sense_ptr = (void *)cmd->frame + ioc->sense_off; + if (in_compat_syscall()) + sense_ptr = compat_ptr((uintptr_t)cmd->frame) + + ioc->sense_off; + else + sense_ptr = (void *)cmd->frame + ioc->sense_off; + if (instance->consistent_mask_64bit) put_unaligned_le64(sense_handle, sense_ptr); else The same might make sense for the iovecs, but I didn't get to that yet.. > static long > megasas_mgmt_compat_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > switch (cmd) { > case MEGASAS_IOC_FIRMWARE32: > - return megasas_mgmt_compat_ioctl_fw(file, arg); > + return megasas_mgmt_ioctl_fw(file, arg); > case MEGASAS_IOC_GET_AEN: > return megasas_mgmt_ioctl_aen(file, arg); > } We should be able to kill off megasas_mgmt_compat_ioctl entirely now.
On Sat, Sep 12, 2020 at 9:48 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Sep 08, 2020 at 11:36:23PM +0200, Arnd Bergmann wrote: > > There have been several attempts to fix serious problems > > in the compat handling in megasas_mgmt_compat_ioctl_fw(), > > and it also uses the compat_alloc_user_space() function. > > I just looked into this a few weeks ago but didn't get that far.. Ok. To make sure we don't needlessly duplicate work, here are the ones I have looked at over the past few days: drivers/scsi/aacraid/linit.c: f = compat_alloc_user_space(sizeof(*f)); drivers/scsi/megaraid/megaraid_sas_base.c: compat_alloc_user_space(sizeof(struct megasas_iocpacket)); Sent out here. drivers/video/fbdev/core/fbmem.c: cmap = compat_alloc_user_space(sizeof(*cmap)); fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct fs_quota_stat)); kernel/kexec.c: ksegments = compat_alloc_user_space(nr_segments * sizeof(out)); mm/mempolicy.c: nm = compat_alloc_user_space(alloc_size); mm/mempolicy.c: nm = compat_alloc_user_space(alloc_size); mm/mempolicy.c: nm = compat_alloc_user_space(alloc_size); mm/mempolicy.c: old = compat_alloc_user_space(new_nodes ? size * 2 : size); mm/mempolicy.c: new = compat_alloc_user_space(size); mm/migrate.c: pages = compat_alloc_user_space(nr_pages * sizeof(void *)); net/socket.c: rxnfc = compat_alloc_user_space(buf_size); net/socket.c: uifr = compat_alloc_user_space(sizeof(*uifr)); sound/core/control_compat.c: data = compat_alloc_user_space(sizeof(*data)); sound/core/hwdep_compat.c: dst = compat_alloc_user_space(sizeof(*dst)); drivers/video/fbdev/sbuslib.c: struct fbcursor __user *p = compat_alloc_user_space(sizeof(*p)); I have patches pending for all the above, but not sent out. drivers/media/v4l2-core/v4l2-compat-ioctl32.c: *new_p64 = compat_alloc_user_space(size + aux_space); Currently looking at this. I have a plan but it's not as easy. drivers/video/fbdev/sbuslib.c: struct fbcmap __user *p = compat_alloc_user_space(sizeof(*p)); I played around with this one but could not come up with a solution that is better than the current one. It's only used on sparc64 though, so we might just leave the interface on that architecture until someone else takes care of it. drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c: karg = compat_alloc_user_space( Had a brief look but did not investigate further, it's complicated. drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = compat_alloc_user_space(sizeof(*args)); drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = compat_alloc_user_space(sizeof(*args) + drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = compat_alloc_user_space(sizeof(*args)); drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = compat_alloc_user_space(sizeof(*args) + drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = compat_alloc_user_space(sizeof(*args)); drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = compat_alloc_user_space(sizeof(*args)); Should not be too hard, but I have not looked in detail. > > +static struct megasas_iocpacket *megasas_compat_iocpacket_get_user(void __user *arg) > > Pointlessly long line. Ok > > +{ > > + int err = -EFAULT; > > +#ifdef CONFIG_COMPAT > > I find the ifdef inside the function a little weird. Doing it in the > caller would be a little less bad. What I ended up doing in my > unfinished patch was to move the compat handling into a new > megaraid_sas_compat.c file, so we'd always get the prototypes in a > header, but given that all the calls are eliminated for the !COMPAT > case we'd avoid ifdefs entirely, but having that file for a single > function is also rather silly. My first attempt was to have no #ifdef at all, but that would require moving "struct compat_iovec" definition outside of the CONFIG_COMPAT check as well. That might be the cleanest though. The reason I ended up with the #ifdef inside of the function was that otherwise I either needed one around the caller and one around the function definition, or an empty inline in the #else path. > > + struct megasas_iocpacket *ioc; > > + struct compat_megasas_iocpacket __user *cioc = arg; > > + int i; > > + > > + ioc = kzalloc(sizeof(*ioc), GFP_KERNEL); > > Missing NULL check here. ok. > > + if (copy_from_user(ioc, arg, > > + offsetof(struct megasas_iocpacket, frame) + 128)) > > + goto out; > > the 128 here while copied from the original code should probably be > replaced with a sizeof(frame->raw). ok. > > + if (ioc->sense_len) { > > + compat_uptr_t *sense_ioc_ptr; > > + void __user *sense_cioc; > > + > > + /* make sure the pointer is inside of frame.raw */ > > + if (ioc->sense_off > > > + (sizeof(ioc->frame.raw) - sizeof(void __user*))) { > > + err = -EINVAL; > > + goto out; > > + } > > + > > + sense_ioc_ptr = (compat_uptr_t *)&ioc->frame.raw[ioc->sense_off]; > > + sense_cioc = compat_ptr(get_unaligned(sense_ioc_ptr)); > > + put_unaligned((unsigned long)sense_cioc, (void **)sense_ioc_ptr); > > I think we should really handle this where the sense point is set up. > This is the untested hunk I had: > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c > index 48fad675b5ed02..c3ddcfce86df50 100644 > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > @@ -8231,7 +8231,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance, > goto out; > } > > - sense_ptr = (void *)cmd->frame + ioc->sense_off; > + if (in_compat_syscall()) > + sense_ptr = compat_ptr((uintptr_t)cmd->frame) + > + ioc->sense_off; > + else > + sense_ptr = (void *)cmd->frame + ioc->sense_off; > + > if (instance->consistent_mask_64bit) > put_unaligned_le64(sense_handle, sense_ptr); > else > > The same might make sense for the iovecs, but I didn't get to that > yet.. I think you got the wrong one there, the code above is where the dma address gets stored in the in-kernel copy of the sense data based on the user space offset. Conceptually it does make sense though and would end up looking something like if (ioc->sense_len) { /* * sense_ptr points to the location that has the user * sense buffer address */ sense_ptr = (void *)ioc->frame.raw + ioc->sense_off; if (in_compat_syscall()) uptr = compat_ptr(get_unaligned(u32 *)sense_ptr); else uptr = get_unaligned((void __user **)sense_ptr); if (copy_to_user(uptr, sense, ioc->sense_len)) { > > static long > > megasas_mgmt_compat_ioctl(struct file *file, unsigned int cmd, > > unsigned long arg) > > { > > switch (cmd) { > > case MEGASAS_IOC_FIRMWARE32: > > - return megasas_mgmt_compat_ioctl_fw(file, arg); > > + return megasas_mgmt_ioctl_fw(file, arg); > > case MEGASAS_IOC_GET_AEN: > > return megasas_mgmt_ioctl_aen(file, arg); > > } > > We should be able to kill off megasas_mgmt_compat_ioctl entirely now. I tried that, but there is still one difference because one of them uses MEGASAS_IOC_FIRMWARE while the other one uses MEGASAS_IOC_FIRMWARE32. It would be possible to have a common handler that always handles both command codes. I tried to avoid changing the behavior that way though. Arnd
On Sat, Sep 12, 2020 at 02:49:05PM +0200, Arnd Bergmann wrote: > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); > fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct > fs_quota_stat)); I sent this out a while ago, an Al has it in a branch, but not in linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.quota-compat > drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c: karg = > compat_alloc_user_space( > > Had a brief look but did not investigate further, it's complicated. > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > compat_alloc_user_space(sizeof(*args)); > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > compat_alloc_user_space(sizeof(*args) + > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > compat_alloc_user_space(sizeof(*args)); > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > compat_alloc_user_space(sizeof(*args) + > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > compat_alloc_user_space(sizeof(*args)); > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > compat_alloc_user_space(sizeof(*args)); > > Should not be too hard, but I have not looked in detail. We do not have to care about staging drivers when removing interfaces. But to be nice you probably ping the maintainers to see what they can do. I also have the mount side handles in this branch which I need to rebase and submit: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/mount-cleanups > I think you got the wrong one there, the code above is where the > dma address gets stored in the in-kernel copy of the sense data > based on the user space offset. Conceptually it does make sense > though and would end up looking something like > > if (ioc->sense_len) { > /* > * sense_ptr points to the location that has the user > * sense buffer address > */ > sense_ptr = (void *)ioc->frame.raw + ioc->sense_off; > if (in_compat_syscall()) > uptr = compat_ptr(get_unaligned(u32 *)sense_ptr); > else > uptr = get_unaligned((void __user **)sense_ptr); > > if (copy_to_user(uptr, sense, ioc->sense_len)) { Indeed. As said, I had started on the change and gave up pretty quickly :) > I tried that, but there is still one difference because one of them uses > MEGASAS_IOC_FIRMWARE while the other one uses > MEGASAS_IOC_FIRMWARE32. It would be possible to have > a common handler that always handles both command codes. > I tried to avoid changing the behavior that way though. Ok.
On Sun, Sep 13, 2020 at 8:50 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Sat, Sep 12, 2020 at 02:49:05PM +0200, Arnd Bergmann wrote: > > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); > > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); > > fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct > > fs_quota_stat)); > > I sent this out a while ago, an Al has it in a branch, but not in > linux-next: > > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.quota-compat Nice! Aside from already being queued, your patch is also nicer than my version, and it makes it trivial to fix it for arm oabi as well by adding #ifdef CONFIG_OABI_COMPAT #define compat_need_64bit_alignment_fixup in_oabi_syscall #endif to arch/arm/include/asm/compat.h I had considered fixing that case for arch/arm as well but it ended up being harder to do in my version. > > drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c: karg = > > compat_alloc_user_space( > > > > Had a brief look but did not investigate further, it's complicated. > > > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > > compat_alloc_user_space(sizeof(*args)); > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > > compat_alloc_user_space(sizeof(*args) + > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > > compat_alloc_user_space(sizeof(*args)); > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > > compat_alloc_user_space(sizeof(*args) + > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > > compat_alloc_user_space(sizeof(*args)); > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args = > > compat_alloc_user_space(sizeof(*args)); > > > > Should not be too hard, but I have not looked in detail. > > We do not have to care about staging drivers when removing interfaces. > But to be nice you probably ping the maintainers to see what they can > do. Right. As both of these are architecture specific, I also considered moving the compat_alloc_user_space() and copy_in_user() definitions for the respective architectures into those drivers and adding the removal into the TODO files. > I also have the mount side handles in this branch which I need to rebase > and submit: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/mount-cleanups I think I had done an almost identical patch for sys_mount() last year and forgotten about it. Again, yours is slightly better ;-) Arnd
On Sun, Sep 13, 2020 at 1:46 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Sep 13, 2020 at 8:50 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Sat, Sep 12, 2020 at 02:49:05PM +0200, Arnd Bergmann wrote: > > > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); > > > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk)); > > > fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct > > > fs_quota_stat)); > > > > I sent this out a while ago, an Al has it in a branch, but not in > > linux-next: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.quota-compat > > Nice! Aside from already being queued, your patch is also nicer than > my version, and it makes it trivial to fix it for arm oabi as well by adding > > #ifdef CONFIG_OABI_COMPAT > #define compat_need_64bit_alignment_fixup in_oabi_syscall > #endif > > to arch/arm/include/asm/compat.h > > I had considered fixing that case for arch/arm as well but it ended up being > harder to do in my version. Unfortunately, the commit b902bfb3f0e "arm64: stop using <asm/compat.h> directly" seems to introduce a circular header file inclusion between linux/compat.h and asm/stat.h, breaking arm64 compilation. Moving the compat_u64/compat_s64 definitions to include/asm-generic/compat.h works fine though. Arnd
On Thu, Sep 17, 2020 at 04:55:49PM +0200, Arnd Bergmann wrote: > Unfortunately, the commit b902bfb3f0e "arm64: stop using <asm/compat.h> > directly" seems to introduce a circular header file inclusion between > linux/compat.h and asm/stat.h, breaking arm64 compilation. > > Moving the compat_u64/compat_s64 definitions to include/asm-generic/compat.h > works fine though. I posted a version doing exactly that a few hours ago.
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c index c3de69f3bee8..601dab468823 100644 --- a/drivers/scsi/megaraid/megaraid_sas_base.c +++ b/drivers/scsi/megaraid/megaraid_sas_base.c @@ -8331,6 +8331,56 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance, return error; } +static struct megasas_iocpacket *megasas_compat_iocpacket_get_user(void __user *arg) +{ + int err = -EFAULT; +#ifdef CONFIG_COMPAT + struct megasas_iocpacket *ioc; + struct compat_megasas_iocpacket __user *cioc = arg; + int i; + + ioc = kzalloc(sizeof(*ioc), GFP_KERNEL); + if (copy_from_user(ioc, arg, + offsetof(struct megasas_iocpacket, frame) + 128)) + goto out; + + /* + * The sense_ptr is used in megasas_mgmt_fw_ioctl only when + * sense_len is not null, so prepare the 64bit value under + * the same condition. + */ + if (ioc->sense_len) { + compat_uptr_t *sense_ioc_ptr; + void __user *sense_cioc; + + /* make sure the pointer is inside of frame.raw */ + if (ioc->sense_off > + (sizeof(ioc->frame.raw) - sizeof(void __user*))) { + err = -EINVAL; + goto out; + } + + sense_ioc_ptr = (compat_uptr_t *)&ioc->frame.raw[ioc->sense_off]; + sense_cioc = compat_ptr(get_unaligned(sense_ioc_ptr)); + put_unaligned((unsigned long)sense_cioc, (void **)sense_ioc_ptr); + } + + for (i = 0; i < MAX_IOCTL_SGE; i++) { + compat_uptr_t iov_base; + if (get_user(iov_base, &cioc->sgl[i].iov_base) || + get_user(ioc->sgl[i].iov_len, &cioc->sgl[i].iov_len)) { + goto out; + } + ioc->sgl[i].iov_base = compat_ptr(iov_base); + } + + return ioc; +out: + kfree(ioc); +#endif + return ERR_PTR(err); +} + static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg) { struct megasas_iocpacket __user *user_ioc = @@ -8339,7 +8389,11 @@ static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg) struct megasas_instance *instance; int error; - ioc = memdup_user(user_ioc, sizeof(*ioc)); + if (in_compat_syscall()) + ioc = megasas_compat_iocpacket_get_user(user_ioc); + else + ioc = memdup_user(user_ioc, sizeof(struct megasas_iocpacket)); + if (IS_ERR(ioc)) return PTR_ERR(ioc); @@ -8444,78 +8498,13 @@ megasas_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) } #ifdef CONFIG_COMPAT -static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) -{ - struct compat_megasas_iocpacket __user *cioc = - (struct compat_megasas_iocpacket __user *)arg; - struct megasas_iocpacket __user *ioc = - compat_alloc_user_space(sizeof(struct megasas_iocpacket)); - int i; - int error = 0; - compat_uptr_t ptr; - u32 local_sense_off; - u32 local_sense_len; - u32 user_sense_off; - - if (clear_user(ioc, sizeof(*ioc))) - return -EFAULT; - - if (copy_in_user(&ioc->host_no, &cioc->host_no, sizeof(u16)) || - copy_in_user(&ioc->sgl_off, &cioc->sgl_off, sizeof(u32)) || - copy_in_user(&ioc->sense_off, &cioc->sense_off, sizeof(u32)) || - copy_in_user(&ioc->sense_len, &cioc->sense_len, sizeof(u32)) || - copy_in_user(ioc->frame.raw, cioc->frame.raw, 128) || - copy_in_user(&ioc->sge_count, &cioc->sge_count, sizeof(u32))) - return -EFAULT; - - /* - * The sense_ptr is used in megasas_mgmt_fw_ioctl only when - * sense_len is not null, so prepare the 64bit value under - * the same condition. - */ - if (get_user(local_sense_off, &ioc->sense_off) || - get_user(local_sense_len, &ioc->sense_len) || - get_user(user_sense_off, &cioc->sense_off)) - return -EFAULT; - - if (local_sense_off != user_sense_off) - return -EINVAL; - - if (local_sense_len) { - void __user **sense_ioc_ptr = - (void __user **)((u8 *)((unsigned long)&ioc->frame.raw) + local_sense_off); - compat_uptr_t *sense_cioc_ptr = - (compat_uptr_t *)(((unsigned long)&cioc->frame.raw) + user_sense_off); - if (get_user(ptr, sense_cioc_ptr) || - put_user(compat_ptr(ptr), sense_ioc_ptr)) - return -EFAULT; - } - - for (i = 0; i < MAX_IOCTL_SGE; i++) { - if (get_user(ptr, &cioc->sgl[i].iov_base) || - put_user(compat_ptr(ptr), &ioc->sgl[i].iov_base) || - copy_in_user(&ioc->sgl[i].iov_len, - &cioc->sgl[i].iov_len, sizeof(compat_size_t))) - return -EFAULT; - } - - error = megasas_mgmt_ioctl_fw(file, (unsigned long)ioc); - - if (copy_in_user(&cioc->frame.hdr.cmd_status, - &ioc->frame.hdr.cmd_status, sizeof(u8))) { - printk(KERN_DEBUG "megasas: error copy_in_user cmd_status\n"); - return -EFAULT; - } - return error; -} - static long megasas_mgmt_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { switch (cmd) { case MEGASAS_IOC_FIRMWARE32: - return megasas_mgmt_compat_ioctl_fw(file, arg); + return megasas_mgmt_ioctl_fw(file, arg); case MEGASAS_IOC_GET_AEN: return megasas_mgmt_ioctl_aen(file, arg); }
There have been several attempts to fix serious problems in the compat handling in megasas_mgmt_compat_ioctl_fw(), and it also uses the compat_alloc_user_space() function. Folding the compat handling into the regular ioctl function with in_compat_syscall() simplifies it a lot and avoids some of the remaining problems: - missing handling of unaligned pointers - overflowing the ioc->frame.raw array from invalid input - compat_alloc_user_space() Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/scsi/megaraid/megaraid_sas_base.c | 123 ++++++++++------------ 1 file changed, 56 insertions(+), 67 deletions(-)