[10/26] bus: mhi: host: use array_size

  Use array_size to protect against multiplication overflows.

The changes were done using the following Coccinelle semantic patch:

// <smpl>
@@
    expression E1, E2;
    constant C1, C2;
    identifier alloc = {vmalloc,vzalloc};
@@
    
(
      alloc(C1 * C2,...)
|
      alloc(
-           (E1) * (E2)
+           array_size(E1, E2)
      ,...)
)
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>

---
 drivers/bus/mhi/host/init.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

On 6/23/2023 3:14 PM, Julia Lawall wrote:
> Use array_size to protect against multiplication overflows.
> 
> The changes were done using the following Coccinelle semantic patch:
> 
> // <smpl>
> @@
>      expression E1, E2;
>      constant C1, C2;
>      identifier alloc = {vmalloc,vzalloc};
> @@
>      
> (
>        alloc(C1 * C2,...)
> |
>        alloc(
> -           (E1) * (E2)
> +           array_size(E1, E2)
>        ,...)
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>
> 
> ---
>   drivers/bus/mhi/host/init.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index f72fcb66f408..34a543a67068 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -759,8 +759,8 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
>   	 * so to avoid any memory possible allocation failures, vzalloc is
>   	 * used here
>   	 */
> -	mhi_cntrl->mhi_chan = vzalloc(mhi_cntrl->max_chan *
> -				      sizeof(*mhi_cntrl->mhi_chan));
> +	mhi_cntrl->mhi_chan = vzalloc(array_size(mhi_cntrl->max_chan,
> +				      sizeof(*mhi_cntrl->mhi_chan)));
>   	if (!mhi_cntrl->mhi_chan)
>   		return -ENOMEM;
>   
> 
> 

This doesn't seem like a good fix.

If we've overflowed the multiplication, I don't think we should 
continue, and the function should return an error.  array_size() is 
going to return SIZE_MAX, and it looks like it is possible that 
vzalloc() may be able to allocate that successfully in some scenarios. 
However, that is going to be less memory than parse_ch_cfg() expected to 
allocate, so later on I expect the function will still corrupt memory - 
basically the same result as what the unchecked overflow would do.

I'm not convinced the semantic patch is bringing value as I suspect most 
of the code being patched is in the same situation.

-Jeff
  

On Fri, 23 Jun 2023, Jeffrey Hugo wrote:

> On 6/23/2023 3:14 PM, Julia Lawall wrote:
> > Use array_size to protect against multiplication overflows.
> >
> > The changes were done using the following Coccinelle semantic patch:
> >
> > // <smpl>
> > @@
> >      expression E1, E2;
> >      constant C1, C2;
> >      identifier alloc = {vmalloc,vzalloc};
> > @@
> >      (
> >        alloc(C1 * C2,...)
> > |
> >        alloc(
> > -           (E1) * (E2)
> > +           array_size(E1, E2)
> >        ,...)
> > )
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>
> >
> > ---
> >   drivers/bus/mhi/host/init.c |    4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > index f72fcb66f408..34a543a67068 100644
> > --- a/drivers/bus/mhi/host/init.c
> > +++ b/drivers/bus/mhi/host/init.c
> > @@ -759,8 +759,8 @@ static int parse_ch_cfg(struct mhi_controller
> > *mhi_cntrl,
> >   	 * so to avoid any memory possible allocation failures, vzalloc is
> >   	 * used here
> >   	 */
> > -	mhi_cntrl->mhi_chan = vzalloc(mhi_cntrl->max_chan *
> > -				      sizeof(*mhi_cntrl->mhi_chan));
> > +	mhi_cntrl->mhi_chan = vzalloc(array_size(mhi_cntrl->max_chan,
> > +				      sizeof(*mhi_cntrl->mhi_chan)));
> >   	if (!mhi_cntrl->mhi_chan)
> >   		return -ENOMEM;
> >
> >
>
> This doesn't seem like a good fix.
>
> If we've overflowed the multiplication, I don't think we should continue, and
> the function should return an error.  array_size() is going to return
> SIZE_MAX, and it looks like it is possible that vzalloc() may be able to
> allocate that successfully in some scenarios. However, that is going to be
> less memory than parse_ch_cfg() expected to allocate, so later on I expect the
> function will still corrupt memory - basically the same result as what the
> unchecked overflow would do.
>
> I'm not convinced the semantic patch is bringing value as I suspect most of
> the code being patched is in the same situation.

OK, this just brings the code in line with all the calls updated by Kees's
original patch, cited in the cover letter, which were all the
calls containing a multiplication that existed at the time.

42bc47b35320 ("treewide: Use array_size() in vmalloc()")
fad953ce0b22 ("treewide: Use array_size() in vzalloc()")

julia

>
> -Jeff
>
  

On 6/23/2023 3:45 PM, Julia Lawall wrote:
> 
> 
> On Fri, 23 Jun 2023, Jeffrey Hugo wrote:
> 
>> On 6/23/2023 3:14 PM, Julia Lawall wrote:
>>> Use array_size to protect against multiplication overflows.
>>>
>>> The changes were done using the following Coccinelle semantic patch:
>>>
>>> // <smpl>
>>> @@
>>>       expression E1, E2;
>>>       constant C1, C2;
>>>       identifier alloc = {vmalloc,vzalloc};
>>> @@
>>>       (
>>>         alloc(C1 * C2,...)
>>> |
>>>         alloc(
>>> -           (E1) * (E2)
>>> +           array_size(E1, E2)
>>>         ,...)
>>> )
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>
>>>
>>> ---
>>>    drivers/bus/mhi/host/init.c |    4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>>> index f72fcb66f408..34a543a67068 100644
>>> --- a/drivers/bus/mhi/host/init.c
>>> +++ b/drivers/bus/mhi/host/init.c
>>> @@ -759,8 +759,8 @@ static int parse_ch_cfg(struct mhi_controller
>>> *mhi_cntrl,
>>>    	 * so to avoid any memory possible allocation failures, vzalloc is
>>>    	 * used here
>>>    	 */
>>> -	mhi_cntrl->mhi_chan = vzalloc(mhi_cntrl->max_chan *
>>> -				      sizeof(*mhi_cntrl->mhi_chan));
>>> +	mhi_cntrl->mhi_chan = vzalloc(array_size(mhi_cntrl->max_chan,
>>> +				      sizeof(*mhi_cntrl->mhi_chan)));
>>>    	if (!mhi_cntrl->mhi_chan)
>>>    		return -ENOMEM;
>>>
>>>
>>
>> This doesn't seem like a good fix.
>>
>> If we've overflowed the multiplication, I don't think we should continue, and
>> the function should return an error.  array_size() is going to return
>> SIZE_MAX, and it looks like it is possible that vzalloc() may be able to
>> allocate that successfully in some scenarios. However, that is going to be
>> less memory than parse_ch_cfg() expected to allocate, so later on I expect the
>> function will still corrupt memory - basically the same result as what the
>> unchecked overflow would do.
>>
>> I'm not convinced the semantic patch is bringing value as I suspect most of
>> the code being patched is in the same situation.
> 
> OK, this just brings the code in line with all the calls updated by Kees's
> original patch, cited in the cover letter, which were all the
> calls containing a multiplication that existed at the time.
> 
> 42bc47b35320 ("treewide: Use array_size() in vmalloc()")
> fad953ce0b22 ("treewide: Use array_size() in vzalloc()")

Eh.  I "git show fad953ce0b22" and it doesn't really tell me much.  The 
commit asserts that uses of vzalloc() and multiplication need 
array_size(), but doesn't really explain why.

This looks like a brute force automated update with no thought and I 
fear the result of this change is the conclusion that we've solved 
multiplication overflow, when it doesn't look like we've really done 
much.  Sure, the multiplication gets capped, but can the code actually 
handle that?

I should probably run the numbers, but with the relevant spec capping 
the number of channels at 256, I don't think we can realistically 
approach overflow, even on a 32-bit system.  However, having correct 
code that is inherently safe seems like a good idea and so I feel this 
function has an issue.  I just don't think this automated conversion 
meaningfully does anything to improve the code here.

Kees, would you please chime in and educate me here?  I feel like I'm 
missing something important here.

-Jeff
  

On Fri, Jun 23, 2023 at 04:09:46PM -0600, Jeffrey Hugo wrote:
> Kees, would you please chime in and educate me here?  I feel like I'm
> missing something important here.

The array_size() family will saturate at SIZE_MAX (rather than potentially
wrapping around). No allocator can fulfil a 18446744073709551615 byte
(18 exabyte) allocation. :) So the NULL return value will (hopefully)
trigger an error path.
  

On 6/23/2023 5:45 PM, Kees Cook wrote:
> On Fri, Jun 23, 2023 at 04:09:46PM -0600, Jeffrey Hugo wrote:
>> Kees, would you please chime in and educate me here?  I feel like I'm
>> missing something important here.
> 
> The array_size() family will saturate at SIZE_MAX (rather than potentially
> wrapping around). No allocator can fulfil a 18446744073709551615 byte
> (18 exabyte) allocation. :) So the NULL return value will (hopefully)
> trigger an error path.
> 

Fair enough, that handles the 64-bit usecase.  I'm guessing the 
assumption is that on a 32-bit usecase where size_t is ~4GB, there won't 
actually be 4GB to allocate and things will also fail.  So far, so good.

What about a 32-bit system with something like ARM's LPAE (Large 
Physical Address Extension) where the host is 32-bit, and so size_t 
would be ~4GB (as far as I can tell) but phys_addr_t is larger than 
that, and so we can have/access more than 4GB of resources?  Lets see, 
ignoring that its a 13 year old feature and probably not in circulation 
anymore, probably still can't satisfy a 4GB allocation since you'd need 
to map all of it to address it, and part of the address space is surely 
reserved for other things.

Ok, I think I'm convinced.  I'm going to sleep on it, but I suspect all 
will still be good early next week.

Thank you for the explanation.

-Jeff
  

On Fri, Jun 23, 2023 at 03:30:36PM -0600, Jeffrey Hugo wrote:
> On 6/23/2023 3:14 PM, Julia Lawall wrote:
> > Use array_size to protect against multiplication overflows.
> > 
> > The changes were done using the following Coccinelle semantic patch:
> > 
> > // <smpl>
> > @@
> >      expression E1, E2;
> >      constant C1, C2;
> >      identifier alloc = {vmalloc,vzalloc};
> > @@
> > (
> >        alloc(C1 * C2,...)
> > |
> >        alloc(
> > -           (E1) * (E2)
> > +           array_size(E1, E2)
> >        ,...)
> > )
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>
> > 
> > ---
> >   drivers/bus/mhi/host/init.c |    4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > index f72fcb66f408..34a543a67068 100644
> > --- a/drivers/bus/mhi/host/init.c
> > +++ b/drivers/bus/mhi/host/init.c
> > @@ -759,8 +759,8 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
> >   	 * so to avoid any memory possible allocation failures, vzalloc is
> >   	 * used here
> >   	 */
> > -	mhi_cntrl->mhi_chan = vzalloc(mhi_cntrl->max_chan *
> > -				      sizeof(*mhi_cntrl->mhi_chan));
> > +	mhi_cntrl->mhi_chan = vzalloc(array_size(mhi_cntrl->max_chan,
> > +				      sizeof(*mhi_cntrl->mhi_chan)));
> >   	if (!mhi_cntrl->mhi_chan)
> >   		return -ENOMEM;
> > 
> > 
> 
> This doesn't seem like a good fix.
> 
> If we've overflowed the multiplication, I don't think we should continue,
> and the function should return an error.  array_size() is going to return
> SIZE_MAX, and it looks like it is possible that vzalloc() may be able to
> allocate that successfully in some scenarios.

Nope.  You can never allocate more that size_t because that's the
highest number that the kernel allocation functions can accept.

Obviously on 64bit size_t is unbelievably large.  If I remember right,
on 32bit you didn't used to be able to allocate more than 2GB without
doing all sorts of tricks.  And everyone deleted those tricks when 64bit
machines became super common.

regards,
dan carpenter
  

On 6/23/2023 3:14 PM, Julia Lawall wrote:
> Use array_size to protect against multiplication overflows.
> 
> The changes were done using the following Coccinelle semantic patch:
> 
> // <smpl>
> @@
>      expression E1, E2;
>      constant C1, C2;
>      identifier alloc = {vmalloc,vzalloc};
> @@
>      
> (
>        alloc(C1 * C2,...)
> |
>        alloc(
> -           (E1) * (E2)
> +           array_size(E1, E2)
>        ,...)
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr>
> 

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>