diff mbox series

[3/3] scsi: megaraid_sas: simplify compat_ioctl handling

Message ID 20200908213715.3553098-3-arnd@arndb.de
State New
Headers show
Series [1/3] scsi: aacraid: improve compat_ioctl handlers | expand

Commit Message

Arnd Bergmann Sept. 8, 2020, 9:36 p.m. UTC
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(-)

-- 
2.27.0

Comments

Christoph Hellwig Sept. 12, 2020, 7:47 a.m. UTC | #1
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.
Arnd Bergmann Sept. 12, 2020, 12:49 p.m. UTC | #2
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
Christoph Hellwig Sept. 13, 2020, 6:50 a.m. UTC | #3
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.
Arnd Bergmann Sept. 13, 2020, 11:46 a.m. UTC | #4
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
Arnd Bergmann Sept. 17, 2020, 2:55 p.m. UTC | #5
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
Christoph Hellwig Sept. 17, 2020, 2:57 p.m. UTC | #6
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 mbox series

Patch

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);
 	}