scsi: aacraid: improve compat_ioctl handlers

PATCH Applied Arnd Bergmann 3 participants 6 messages Oct 30, 2020 – Nov 11, 2020

Subsystems (4)

Stable branch SCSI Megaraid SCSI/SAS drivers Aacraid SCSI RAID driver

lore

Share & tools

lore

Arnd Bergmann

author

2020-10-30 16:45 UTC #0 lore raw

PATCH v3 1/3 scsi: aacraid: improve compat_ioctl handlers

From: Arnd Bergmann <[email protected]>

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")

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

---
 drivers/scsi/aacraid/commctrl.c | 22 ++++++++++--
 drivers/scsi/aacraid/linit.c    | 61 ++-------------------------------
 2 files changed, 22 insertions(+), 61 deletions(-)

drivers/scsi/aacraid/commctrl.c +20 -2

index e3e157a74988..1b1da162f5f6 100644
@@ -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>

@@ -226,6 +227,12 @@ static int open_getadapter_fib(struct aac_dev * dev, void __user *arg)
 	return status;
 }
 
+struct compat_fib_ioctl {
+	u32	fibctx;
+	s32	wait;
+	compat_uptr_t fib;
+};
+
 /**
  *	next_getadapter_fib	-	get the next fib
  *	@dev: adapter to use

@@ -243,8 +250,19 @@ 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 cf;
+
+		if (copy_from_user(&cf, 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
 	 *

drivers/scsi/aacraid/linit.c +2 ~~-59~~

79 lines not on this page · raw message

Signature · 1 line

2.27.0

Arnd Bergmann

author

2020-10-30 16:45 UTC #1 lore raw

PATCH v3 2/3 scsi: megaraid_sas: check user-provided offsets

From: Arnd Bergmann <[email protected]>

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.

Cc <[email protected]> # v2.6.15+

Fixes c4a3e0a529ab ("[SCSI] MegaRAID SAS RAID: new driver")

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

---
 drivers/scsi/megaraid/megaraid_sas_base.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

drivers/scsi/megaraid/megaraid_sas_base.c +10 -5

index 41cd66fc7d81..b1b9a8823c8c 100644
@@ -8134,7 +8134,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	int error = 0, i;
 	void *sense = NULL;
 	dma_addr_t sense_handle;
-	unsigned long *sense_ptr;
+	void *sense_ptr;
 	u32 opcode = 0;
 	int ret = DCMD_SUCCESS;

@@ -8257,6 +8257,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	}
 
 	if (ioc->sense_len) {
+		/* make sure the pointer is part of the frame */
+		if (ioc->sense_off > (sizeof(union megasas_frame) - sizeof(__le64))) {
+			error = -EINVAL;
+			goto out;
+		}
+
 		sense = dma_alloc_coherent(&instance->pdev->dev, ioc->sense_len,
 					     &sense_handle, GFP_KERNEL);
 		if (!sense) {

@@ -8264,12 +8270,11 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 			goto out;
 		}
 
-		sense_ptr =
-		(unsigned long *) ((unsigned long)cmd->frame + ioc->sense_off);
+		sense_ptr = (void *)cmd->frame + ioc->sense_off;
 		if (instance->consistent_mask_64bit)
-			*sense_ptr = cpu_to_le64(sense_handle);
+			put_unaligned_le64(sense_handle, sense_ptr);
 		else
-			*sense_ptr = cpu_to_le32(sense_handle);
+			put_unaligned_le32(sense_handle, sense_ptr);
 	}
 
 	/*

Signature · 1 line

2.27.0

Christoph Hellwig

2020-11-03 08:55 UTC #2 lore raw

Re: [PATCH v3 2/3] scsi: megaraid_sas: check user-provided offsets

Quoted 34 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
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.
Cc <[email protected]> # v2.6.15+
Fixes c4a3e0a529ab ("[SCSI] MegaRAID SAS RAID: new driver")
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Arnd Bergmann <[email protected]>
---
 drivers/scsi/megaraid/megaraid_sas_base.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/scsi/megaraid/megaraid_sas_base.c +1 -1
index 41cd66fc7d81..b1b9a8823c8c 100644
@@ -8134,7 +8134,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	int error = 0, i;
 	void *sense = NULL;
 	dma_addr_t sense_handle;
-	unsigned long *sense_ptr;
+	void *sense_ptr;
 	u32 opcode = 0;
 	int ret = DCMD_SUCCESS;
 

drivers/scsi/megaraid/megaraid_sas_base.c megasas_mgmt_fw_ioctl(struct megasas_instance *instance, · line 8262

Quoted 6 lines

drivers/scsi/megaraid/megaraid_sas_base.c +3 -0

@@ -8257,6 +8257,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	}
 
 	if (ioc->sense_len) {
+		/* make sure the pointer is part of the frame */
+		if (ioc->sense_off > (sizeof(union megasas_frame) - sizeof(__le64))) {
+			error = -EINVAL;

This still has the overly long line.

Arnd Bergmann

author

2020-10-30 16:45 UTC #3 lore raw

PATCH v3 3/3 scsi: megaraid_sas: simplify compat_ioctl handling

In reply to Arnd Bergmann

From: Arnd Bergmann <[email protected]>

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 <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
v3: address even more review comments from hch
v2: address review comments from hch

---
 drivers/scsi/megaraid/megaraid_sas.h      |   2 -
 drivers/scsi/megaraid/megaraid_sas_base.c | 117 +++++++++-------------
 2 files changed, 45 insertions(+), 74 deletions(-)

drivers/scsi/megaraid/megaraid_sas.h +0 -2

index 5e4137f10e0e..0f808d63580e 100644
@@ -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)

drivers/scsi/megaraid/megaraid_sas_base.c +45 ~~-72~~

152 lines not on this page · raw message

Signature · 1 line

2.27.0

Martin K. Petersen

Applied

2020-11-05 02:57 UTC #4 lore raw

In reply to Arnd Bergmann

Arnd,

Quoted 4 lines

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.

Applied to 5.11/scsi-staging, thanks!

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Martin K. Petersen

Applied

2020-11-11 02:58 UTC #5 lore raw

In reply to Arnd Bergmann

Quoted 9 lines · Arnd Bergmann

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.

Applied to 5.11/scsi-queue, thanks!

[1/3] scsi: aacraid: Improve compat_ioctl handlers

      https://git.kernel.org/mkp/scsi/c/077054215a7f
[2/3] scsi: megaraid_sas: Check user-provided offsets
      https://git.kernel.org/mkp/scsi/c/381d34e376e3

[3/3] scsi: megaraid_sas: Simplify compat_ioctl handling
https://git.kernel.org/mkp/scsi/c/bba84aeccafb

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering