Skip to contentSkip to conversation

drivers/scsi/mpt2sas: prevent heap overflows and unchecked reads

PATCH In review Dan Rosenberg 2 participants 2 messages Apr 5, 2011 – Apr 14, 2011 SCSI

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 drivers/scsi/mpt2sas: prevent heap overflows and unchecked reads

At two points in handling device ioctls via /dev/mpt2ctl, user-supplied length values are used to copy data from userspace into heap buffers without bounds checking, allowing controllable heap corruption and subsequently privilege escalation.

Additionally, user-supplied values are used to determine the size of a copy_to_user() as well as the offset into the buffer to be read, with no bounds checking, allowing users to read arbitrary kernel memory.

Compile-tested only. Thanks to Eric Moore for suggestions on how to fix this.

---
 drivers/scsi/mpt2sas/mpt2sas_ctl.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)
drivers/scsi/mpt2sas/mpt2sas_ctl.c +21 -2
index e92b77a..3834c95 100644
@@ -688,6 +688,13 @@ _ctl_do_mpt_command(struct MPT2SAS_ADAPTER *ioc,
 		goto out;
 	}
 
+	/* Check for overflow and wraparound */
+	if (karg.data_sge_offset * 4 > ioc->request_sz ||
+	    karg.data_sge_offset > (UINT_MAX / 4)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* copy in request message frame from user */
 	if (copy_from_user(mpi_request, mf, karg.data_sge_offset*4)) {
 		printk(KERN_ERR "failure at %s:%d/%s()!\n", __FILE__, __LINE__,
@@ -1963,7 +1970,7 @@ _ctl_diag_read_buffer(void __user *arg, enum block_state state)
 	Mpi2DiagBufferPostReply_t *mpi_reply;
 	int rc, i;
 	u8 buffer_type;
-	unsigned long timeleft;
+	unsigned long timeleft, request_size, copy_size;
 	u16 smid;
 	u16 ioc_status;
 	u8 issue_reset = 0;
@@ -1999,6 +2006,8 @@ _ctl_diag_read_buffer(void __user *arg, enum block_state state)
 		return -ENOMEM;
 	}
 
+	request_size = ioc->diag_buffer_sz[buffer_type];
+
 	if ((karg.starting_offset % 4) || (karg.bytes_to_read % 4)) {
 		printk(MPT2SAS_ERR_FMT "%s: either the starting_offset "
 		    "or bytes_to_read are not 4 byte aligned\n", ioc->name,
@@ -2006,13 +2015,23 @@ _ctl_diag_read_buffer(void __user *arg, enum block_state state)
 		return -EINVAL;
 	}
 
+	if (karg.starting_offset > request_size)
+		return -EINVAL;
+
 	diag_data = (void *)(request_data + karg.starting_offset);
 	dctlprintk(ioc, printk(MPT2SAS_INFO_FMT "%s: diag_buffer(%p), "
 	    "offset(%d), sz(%d)\n", ioc->name, __func__,
 	    diag_data, karg.starting_offset, karg.bytes_to_read));
 
+	/* Truncate data on requests that are too large */
+	if ((diag_data + karg.bytes_to_read < diag_data) ||
+	    (diag_data + karg.bytes_to_read > request_data + request_size))
+		copy_size = request_size - karg.starting_offset;
+	else
+		copy_size = karg.bytes_to_read;
+
 	if (copy_to_user((void __user *)uarg->diagnostic_data,
-	    diag_data, karg.bytes_to_read)) {
+	    diag_data, copy_size)) {
 		printk(MPT2SAS_ERR_FMT "%s: Unable to write "
 		    "mpt_diag_read_buffer_t data @ %p\n", ioc->name,
 		    __func__, diag_data);
#1 lore raw

This patch has been tested by Eric Moore and it works fine.

Please consider this patch "Acked-by: Eric Moore <[email protected]>"

Thanks, Kashyap

Quoted 97 lines

-----Original Message-----
From: Dan Rosenberg [mailto:[email protected]]
Sent: Tuesday, April 05, 2011 10:16 PM
To: Moore, Eric; [email protected]; Desai, Kashyap
Cc: [email protected]; [email protected]; linux- [email protected]
Subject: [PATCH] drivers/scsi/mpt2sas: prevent heap overflows and unchecked reads

At two points in handling device ioctls via /dev/mpt2ctl, user-supplied length values are used to copy data from userspace into heap buffers without bounds checking, allowing controllable heap corruption and subsequently privilege escalation.

Additionally, user-supplied values are used to determine the size of a copy_to_user() as well as the offset into the buffer to be read, with no
bounds checking, allowing users to read arbitrary kernel memory.

Compile-tested only. Thanks to Eric Moore for suggestions on how to fix
this.

---
 drivers/scsi/mpt2sas/mpt2sas_ctl.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)
drivers/scsi/mpt2sas/mpt2sas_ctl.c +7 -0
index e92b77a..3834c95 100644
@@ -688,6 +688,13 @@ _ctl_do_mpt_command(struct MPT2SAS_ADAPTER *ioc,
 		goto out;
 	}
 
+	/* Check for overflow and wraparound */
+	if (karg.data_sge_offset * 4 > ioc->request_sz ||
+	    karg.data_sge_offset > (UINT_MAX / 4)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* copy in request message frame from user */
 	if (copy_from_user(mpi_request, mf, karg.data_sge_offset*4)) {
 		printk(KERN_ERR "failure at %s:%d/%s()!\n", __FILE__,

__LINE__,

drivers/scsi/mpt2sas/mpt2sas_ctl.c +1 -1
@@ -1963,7 +1970,7 @@ _ctl_diag_read_buffer(void __user *arg, enum
 block_state state)
 	Mpi2DiagBufferPostReply_t *mpi_reply;
 	int rc, i;
 	u8 buffer_type;
-	unsigned long timeleft;
+	unsigned long timeleft, request_size, copy_size;
 	u16 smid;
 	u16 ioc_status;
drivers/scsi/mpt2sas/mpt2sas_ctl.c +2 -0
 	u8 issue_reset = 0;
@@ -1999,6 +2006,8 @@ _ctl_diag_read_buffer(void __user *arg, enum
 block_state state)
 		return -ENOMEM;
 	}
 
+	request_size = ioc->diag_buffer_sz[buffer_type];
+
 	if ((karg.starting_offset % 4) || (karg.bytes_to_read % 4)) {
 		printk(MPT2SAS_ERR_FMT "%s: either the starting_offset "
drivers/scsi/mpt2sas/mpt2sas_ctl.c +11 -1
 		    "or bytes_to_read are not 4 byte aligned\n", ioc->name,
@@ -2006,13 +2015,23 @@ _ctl_diag_read_buffer(void __user *arg, enum
 block_state state)
 		return -EINVAL;
 	}
 
+	if (karg.starting_offset > request_size)
+		return -EINVAL;
+
 	diag_data = (void *)(request_data + karg.starting_offset);
 	dctlprintk(ioc, printk(MPT2SAS_INFO_FMT "%s: diag_buffer(%p), "
 	    "offset(%d), sz(%d)\n", ioc->name, __func__,
 	    diag_data, karg.starting_offset, karg.bytes_to_read));
 
+	/* Truncate data on requests that are too large */
+	if ((diag_data + karg.bytes_to_read < diag_data) ||
+	    (diag_data + karg.bytes_to_read > request_data +
 request_size))
+		copy_size = request_size - karg.starting_offset;
+	else
+		copy_size = karg.bytes_to_read;
+
 	if (copy_to_user((void __user *)uarg->diagnostic_data,
-	    diag_data, karg.bytes_to_read)) {
+	    diag_data, copy_size)) {
 		printk(MPT2SAS_ERR_FMT "%s: Unable to write "

drivers/scsi/mpt2sas/mpt2sas_ctl.c _ctl_diag_read_buffer(void __user *arg, enum block_state state) · line 2037

Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_ctl.c +0 -0
 		    "mpt_diag_read_buffer_t data @ %p\n", ioc->name,
 		    __func__, diag_data);

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�