| Message ID | 1306184271-5388-1-git-send-email-herton.krzesinski@canonical.com |
|---|---|
| State | New |
| Headers | show |
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()!\n", __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\n", 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)\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);
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()!\n", __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\n", 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)\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); Acked-by: Stefan Bader <stefan.bader@canonical.com>
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()!\n", __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\n", 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()!\n", __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.
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()!\n", __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\n", 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()!\n", __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
Applied On Mon, 2011-05-23 at 17:57 -0300, 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()!\n", __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\n", 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)\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.7.4.1 > >
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()!\n", __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\n", 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)\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);