[CVE-2011-1494][Maverick][PATCH] mpt2sas: prevent heap overflows and unchecked reads, CVE-2011-1494

[John Johansen]

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