diff mbox series

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

Message ID 20200918121543.1466090-1-arnd@arndb.de (mailing list archive)
State Changes Requested
Headers show
Series [v2,1/3] scsi: aacraid: improve compat_ioctl handlers | expand

Commit Message

Arnd Bergmann Sept. 18, 2020, 12:15 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>
---
v2: address review comments from hch
---
 drivers/scsi/megaraid/megaraid_sas.h      |   2 -
 drivers/scsi/megaraid/megaraid_sas_base.c | 117 +++++++++-------------
 include/linux/compat.h                    |  10 +-
 3 files changed, 50 insertions(+), 79 deletions(-)

Comments

Christoph Hellwig Sept. 19, 2020, 5:26 a.m. UTC | #1
On Fri, Sep 18, 2020 at 02:15:43PM +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.
> 
> 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>
> ---
> v2: address review comments from hch
> ---
>  drivers/scsi/megaraid/megaraid_sas.h      |   2 -
>  drivers/scsi/megaraid/megaraid_sas_base.c | 117 +++++++++-------------
>  include/linux/compat.h                    |  10 +-
>  3 files changed, 50 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
> index 5e4137f10e0e..0f808d63580e 100644
> --- a/drivers/scsi/megaraid/megaraid_sas.h
> +++ b/drivers/scsi/megaraid/megaraid_sas.h
> @@ -2605,7 +2605,6 @@ struct megasas_aen {
>  	u32 class_locale_word;
>  } __attribute__ ((packed));
>  
> -#ifdef CONFIG_COMPAT
>  struct compat_megasas_iocpacket {
>  	u16 host_no;
>  	u16 __pad1;
> @@ -2621,7 +2620,6 @@ struct compat_megasas_iocpacket {
>  } __attribute__ ((packed));
>  
>  #define MEGASAS_IOC_FIRMWARE32	_IOWR('M', 1, struct compat_megasas_iocpacket)
> -#endif
>  
>  #define MEGASAS_IOC_FIRMWARE	_IOWR('M', 1, struct megasas_iocpacket)
>  #define MEGASAS_IOC_GET_AEN	_IOW('M', 3, struct megasas_aen)
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index c3de69f3bee8..d91951ee16ab 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -8279,16 +8279,18 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
>  	 * copy out the sense
>  	 */
>  	if (ioc->sense_len) {
> +		void __user *uptr;
>  		/*
>  		 * 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));

should the u32 * here by a compat_uptr *? Not tat it would make a
difference, just better document what we are doing.

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

I don't think we need the braces here.

> +	return ioc;
> +out:
> +	kfree(ioc);
> +
> +	return ERR_PTR(err);

spurious empty line.

> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -91,6 +91,11 @@
>  	static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
>  #endif /* COMPAT_SYSCALL_DEFINEx */
>  
> +struct compat_iovec {
> +	compat_uptr_t	iov_base;
> +	compat_size_t	iov_len;
> +};
> +
>  #ifdef CONFIG_COMPAT
>  
>  #ifndef compat_user_stack_pointer
> @@ -248,11 +253,6 @@ typedef struct compat_siginfo {
>  	} _sifields;
>  } compat_siginfo_t;
>  
> -struct compat_iovec {
> -	compat_uptr_t	iov_base;
> -	compat_size_t	iov_len;
> -};

This should probably go into a separate patch instead of being hidden
in a driver patch.

But except for these nitpicks the change looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Arnd Bergmann Sept. 26, 2020, 9:19 p.m. UTC | #2
On Sat, Sep 19, 2020 at 7:26 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Sep 18, 2020 at 02:15:43PM +0200, Arnd Bergmann wrote:
> > @@ -8279,16 +8279,18 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
> >        * copy out the sense
> >        */
> >       if (ioc->sense_len) {
> > +             void __user *uptr;
> >               /*
> >                * 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));
>
> should the u32 * here by a compat_uptr *? Not tat it would make a
> difference, just better document what we are doing.

Ok, changed now. The longer type name leads to a slightly ugly
line break, but you are right that is is the correct thing to do.

> > +     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;
> > +             }
>
> I don't think we need the braces here.

ok.

> > +     return ioc;
> > +out:
> > +     kfree(ioc);
> > +
> > +     return ERR_PTR(err);
>
> spurious empty line.

ok

     Arnd
Joe Perches Sept. 27, 2020, 4:52 a.m. UTC | #3
On Sat, 2020-09-26 at 23:19 +0200, Arnd Bergmann wrote:
> On Sat, Sep 19, 2020 at 7:26 AM Christoph Hellwig <hch@infradead.org> wrote:
> > On Fri, Sep 18, 2020 at 02:15:43PM +0200, Arnd Bergmann wrote:
[]
> 
> > > +     return ioc;
> > > +out:
> > > +     kfree(ioc);
> > > +
> > > +     return ERR_PTR(err);
> > 
> > spurious empty line.
> 
> ok

I suggest moving the blank line so it's between
the return ioc and out: label.
diff mbox series

Patch

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index 5e4137f10e0e..0f808d63580e 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -2605,7 +2605,6 @@  struct megasas_aen {
 	u32 class_locale_word;
 } __attribute__ ((packed));
 
-#ifdef CONFIG_COMPAT
 struct compat_megasas_iocpacket {
 	u16 host_no;
 	u16 __pad1;
@@ -2621,7 +2620,6 @@  struct compat_megasas_iocpacket {
 } __attribute__ ((packed));
 
 #define MEGASAS_IOC_FIRMWARE32	_IOWR('M', 1, struct compat_megasas_iocpacket)
-#endif
 
 #define MEGASAS_IOC_FIRMWARE	_IOWR('M', 1, struct megasas_iocpacket)
 #define MEGASAS_IOC_GET_AEN	_IOW('M', 3, struct megasas_aen)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index c3de69f3bee8..d91951ee16ab 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -8279,16 +8279,18 @@  megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	 * copy out the sense
 	 */
 	if (ioc->sense_len) {
+		void __user *uptr;
 		/*
 		 * sense_ptr points to the location that has the user
 		 * sense buffer address
 		 */
-		sense_ptr = (unsigned long *) ((unsigned long)ioc->frame.raw +
-				ioc->sense_off);
+		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((void __user *)((unsigned long)
-				 get_unaligned((unsigned long *)sense_ptr)),
-				 sense, ioc->sense_len)) {
+		if (copy_to_user(uptr, sense, ioc->sense_len)) {
 			dev_err(&instance->pdev->dev, "Failed to copy out to user "
 					"sense data\n");
 			error = -EFAULT;
@@ -8331,6 +8333,38 @@  megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	return error;
 }
 
+static struct megasas_iocpacket *
+megasas_compat_iocpacket_get_user(void __user *arg)
+{
+	struct megasas_iocpacket *ioc;
+	struct compat_megasas_iocpacket __user *cioc = arg;
+	size_t size;
+	int err = -EFAULT;
+	int i;
+
+	ioc = kzalloc(sizeof(*ioc), GFP_KERNEL);
+	if (!ioc)
+		return ERR_PTR(-ENOMEM);
+	size = offsetof(struct megasas_iocpacket, frame) + sizeof(ioc->frame);
+	if (copy_from_user(ioc, arg, size))
+		goto out;
+
+	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);
+
+	return ERR_PTR(err);
+}
+
 static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg)
 {
 	struct megasas_iocpacket __user *user_ioc =
@@ -8339,7 +8373,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 +8482,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);
 	}
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 1a530e1aa15a..a7a5a0ff59ef 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -91,6 +91,11 @@ 
 	static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
 #endif /* COMPAT_SYSCALL_DEFINEx */
 
+struct compat_iovec {
+	compat_uptr_t	iov_base;
+	compat_size_t	iov_len;
+};
+
 #ifdef CONFIG_COMPAT
 
 #ifndef compat_user_stack_pointer
@@ -248,11 +253,6 @@  typedef struct compat_siginfo {
 	} _sifields;
 } compat_siginfo_t;
 
-struct compat_iovec {
-	compat_uptr_t	iov_base;
-	compat_size_t	iov_len;
-};
-
 struct compat_rlimit {
 	compat_ulong_t	rlim_cur;
 	compat_ulong_t	rlim_max;