FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 05-23-2011, 11:32 PM
John Johansen
 
Default mpt2sas: prevent heap overflows and unchecked reads, CVE-2011-1494

On 05/23/2011 01:57 PM, Herton Ronaldo Krzesinski wrote:
> From: Dan Rosenberg <drosenberg@vsecurity.com>
>
> CVE-2011-1494
>
> BugLink: http://bugs.launchpad.net/bugs/787145
>
> Released until now with stable versions 2.6.32.40, 2.6.33.13, 2.6.38.6
>
> 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.
>
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Cc: stable@kernel.org
> Acked-by: Eric Moore <eric.moore@lsi.com>
> Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> (backported from commit a1f74ae82d133ebb2aabb19d181944b4e83e9960 upstream)
> Signed-off-by: Herton Krzesinski <herton.krzesinski@canonical.com>

Acked-by: John Johansen <john.johansen@canonical.com>

> ---
> drivers/scsi/mpt2sas/mpt2sas_ctl.c | 23 +++++++++++++++++++++--
> 1 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_ctl.c b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> index d88e975..9e689c8 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> @@ -637,6 +637,13 @@ _ctl_do_mpt_command(struct MPT2SAS_ADAPTER *ioc,
> data_out_sz = karg.data_out_size;
> data_in_sz = karg.data_in_size;
>
> + /* 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()!
", __FILE__, __LINE__,
> @@ -1883,7 +1890,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;
> @@ -1919,6 +1926,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
", ioc->name,
> @@ -1926,13 +1935,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_DEBUG_FMT "%s: diag_buffer(%p), "
> "offset(%d), sz(%d)
", 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
", ioc->name,
> __func__, diag_data);


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 05-24-2011, 07:28 AM
Stefan Bader
 
Default mpt2sas: prevent heap overflows and unchecked reads, CVE-2011-1494

On 23.05.2011 22:57, Herton Ronaldo Krzesinski wrote:

From: Dan Rosenberg<drosenberg@vsecurity.com>

CVE-2011-1494

BugLink: http://bugs.launchpad.net/bugs/787145

Released until now with stable versions 2.6.32.40, 2.6.33.13, 2.6.38.6

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.

Signed-off-by: Dan Rosenberg<drosenberg@vsecurity.com>
Cc: stable@kernel.org
Acked-by: Eric Moore<eric.moore@lsi.com>
Signed-off-by: James Bottomley<James.Bottomley@suse.de>
(backported from commit a1f74ae82d133ebb2aabb19d181944b4e83e9960 upstream)
Signed-off-by: Herton Krzesinski<herton.krzesinski@canonical.com>
---
drivers/scsi/mpt2sas/mpt2sas_ctl.c | 23 +++++++++++++++++++++--
1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt2sas/mpt2sas_ctl.c b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
index d88e975..9e689c8 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_ctl.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
@@ -637,6 +637,13 @@ _ctl_do_mpt_command(struct MPT2SAS_ADAPTER *ioc,
data_out_sz = karg.data_out_size;
data_in_sz = karg.data_in_size;

+ /* 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()!
", __FILE__, __LINE__,
@@ -1883,7 +1890,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;
@@ -1919,6 +1926,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
", ioc->name,
@@ -1926,13 +1935,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_DEBUG_FMT "%s: diag_buffer(%p), "
"offset(%d), sz(%d)
", 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
", ioc->name,
__func__, diag_data);

Acked-by: Stefan Bader <stefan.bader@canonical.com>

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 05-24-2011, 08:23 AM
Tetsuo Handa
 
Default mpt2sas: prevent heap overflows and unchecked reads, CVE-2011-1494

Stefan Bader wrote:
> > --- a/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> > +++ b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
> > @@ -637,6 +637,13 @@ _ctl_do_mpt_command(struct MPT2SAS_ADAPTER *ioc,
> > data_out_sz = karg.data_out_size;
> > data_in_sz = karg.data_in_size;
> >
> > + /* 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()!
", __FILE__, __LINE__,

karg.data_sge_offset is assigned from user-supplied memory.
I don't know whether ioc->request_sz can be set to 0.

But if ioc->request_sz can be set to 0,

683 mpi_request = kzalloc(ioc->request_sz, GFP_KERNEL);

mpi_request is set to ZERO_SIZE_PTR

684 if (!mpi_request) {
685 printk(MPT2SAS_ERR_FMT "%s: failed obtaining a memory for "
686 "mpi_request
", ioc->name, __func__);
687 ret = -ENOMEM;
688 goto out;
689 }
690

and reaches here. If karg.data_sge_offset == 0,

691 /* Check for overflow and wraparound */
692 if (karg.data_sge_offset * 4 > ioc->request_sz ||
693 karg.data_sge_offset > (UINT_MAX / 4)) {
694 ret = -EINVAL;
695 goto out;
696 }
697

reaches here.

copy_from_user() copies no data, but

698 /* copy in request message frame from user */
699 if (copy_from_user(mpi_request, mf, karg.data_sge_offset*4)) {
700 printk(KERN_ERR "failure at %s:%d/%s()!
", __FILE__, __LINE__,
701 __func__);
702 ret = -EFAULT;
703 goto out;
704 }
705

accessing ZERO_SIZE_PTR->Function is an oops.

706 if (mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT) {

Many functions are passing a variable to (e.g.) kmalloc() with an
assumption that the value of that variable is never 0.

Although this was once NACKed at http://lkml.org/lkml/2010/2/26/30 , don't we
want memory allocator functions that return NULL if requested size is 0?


At line 706, mpi_request->Function is always 0 if karg.data_sge_offset == 0 and
ioc->request_sz != 0 ... Maybe we need karg.data_sge_offset != 0 check.

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 05-24-2011, 09:35 AM
John Johansen
 
Default mpt2sas: prevent heap overflows and unchecked reads, CVE-2011-1494

On 05/24/2011 01:23 AM, Tetsuo Handa wrote:
> Stefan Bader wrote:
>>> --- a/drivers/scsi/mpt2sas/mpt2sas_ctl.c
>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_ctl.c
>>> @@ -637,6 +637,13 @@ _ctl_do_mpt_command(struct MPT2SAS_ADAPTER *ioc,
>>> data_out_sz = karg.data_out_size;
>>> data_in_sz = karg.data_in_size;
>>>
>>> + /* 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()!
", __FILE__, __LINE__,
>
> karg.data_sge_offset is assigned from user-supplied memory.
> I don't know whether ioc->request_sz can be set to 0.
>
Right, I did poke through at this and didn't come up with a satisfactory answer.
I need to spend more time with the code to figure that case out.

> But if ioc->request_sz can be set to 0,
>
> 683 mpi_request = kzalloc(ioc->request_sz, GFP_KERNEL);
>
> mpi_request is set to ZERO_SIZE_PTR
>
> 684 if (!mpi_request) {
> 685 printk(MPT2SAS_ERR_FMT "%s: failed obtaining a memory for "
> 686 "mpi_request
", ioc->name, __func__);
> 687 ret = -ENOMEM;
> 688 goto out;
> 689 }
> 690
>
> and reaches here. If karg.data_sge_offset == 0,
>
> 691 /* Check for overflow and wraparound */
> 692 if (karg.data_sge_offset * 4 > ioc->request_sz ||
> 693 karg.data_sge_offset > (UINT_MAX / 4)) {
> 694 ret = -EINVAL;
> 695 goto out;
> 696 }
> 697
>
> reaches here.
>
> copy_from_user() copies no data, but
>
> 698 /* copy in request message frame from user */
> 699 if (copy_from_user(mpi_request, mf, karg.data_sge_offset*4)) {
> 700 printk(KERN_ERR "failure at %s:%d/%s()!
", __FILE__, __LINE__,
> 701 __func__);
> 702 ret = -EFAULT;
> 703 goto out;
> 704 }
> 705
>
> accessing ZERO_SIZE_PTR->Function is an oops.
>
yep

> 706 if (mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT) {
>
> Many functions are passing a variable to (e.g.) kmalloc() with an
> assumption that the value of that variable is never 0.
>
> Although this was once NACKed at http://lkml.org/lkml/2010/2/26/30 , don't we
> want memory allocator functions that return NULL if requested size is 0?
>
while I agree with you, and would actually prefer to see kmalloc style functions
that can return ZERO_SIZE_PTR as wrappers around the kernels kmalloc, and code
that actually needs that feature call them explicitly. I think we are stuck with
the current behavior.

>
> At line 706, mpi_request->Function is always 0 if karg.data_sge_offset == 0 and
> ioc->request_sz != 0 ... Maybe we need karg.data_sge_offset != 0 check.
>

That is a check that I am not comfortable making with out having better
understanding of what the code is doing. From my cursory reading of the code
I would say that karg.data_sge_offset == 0 might be a valid value. Its something
that really needs to be taken up with the maintainer.

From an SRU pov, the patch is a clean back port of an upstream patch and it does
address the bug, so it is worth taking in its current form.

thanks Tetsuo for spending time doing your review



--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 08:36 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org