[PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument

[PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument

[sayli karnik]

This patch finds cases where the argument to sizeof is wrong in memory
allocation functions by checking the type of the allocated memory when it is a
double pointer and ensuring the sizeof argument takes a pointer to the memory
being allocated.
Done using coccinelle:
@@
type T;
T **x;
@@

  x =
  <+...sizeof(
- T
+ *x
  )...+>

Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
---
 drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
index 659aa71..9f9b244 100644
--- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c
+++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
@@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev)
 		return PTR_ERR(ipdev->data);
 
 	ipdev->heaps = devm_kzalloc(&pdev->dev,
-				sizeof(struct ion_heap)*ipdev->data->nr,
+				sizeof(*ipdev->heaps) * ipdev->data->nr,
 				GFP_KERNEL);
 	if (!ipdev->heaps) {
 		ion_destroy_platform_data(ipdev->data);
-- 
2.7.4

Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument

[Greg Kroah-Hartman]

On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote:
> This patch finds cases where the argument to sizeof is wrong in memory
> allocation functions by checking the type of the allocated memory when it is a
> double pointer and ensuring the sizeof argument takes a pointer to the memory
> being allocated.

Are you sure?


> Done using coccinelle:
> @@
> type T;
> T **x;
> @@
> 
>   x =
>   <+...sizeof(
> - T
> + *x
>   )...+>
> 
> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> ---
>  drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> index 659aa71..9f9b244 100644
> --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> +++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> @@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev)
>  		return PTR_ERR(ipdev->data);
>  
>  	ipdev->heaps = devm_kzalloc(&pdev->dev,
> -				sizeof(struct ion_heap)*ipdev->data->nr,
> +				sizeof(*ipdev->heaps) * ipdev->data->nr,

Do we really want to allocate an array of pointers, or an array of
structures?  Which is it, as it's not obvious...

thanks,

greg k-h

Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument

[sayli karnik]

On Wed, Sep 21, 2016 at 9:45 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote:
>> This patch finds cases where the argument to sizeof is wrong in memory
>> allocation functions by checking the type of the allocated memory when it is a
>> double pointer and ensuring the sizeof argument takes a pointer to the memory
>> being allocated.
>
> Are you sure?
>
This was detected by scripts/coccinelle/misc/badty.cocci, which
specifies that a false positive only arises when the sizeof argument
is not used in constructing the return value. Also the driver builds
without errors. What do you think?

>
>> Done using coccinelle:
>> @@
>> type T;
>> T **x;
>> @@
>>
>>   x =
>>   <+...sizeof(
>> - T
>> + *x
>>   )...+>
>>
>> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
>> ---
>>  drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
>> index 659aa71..9f9b244 100644
>> --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c
>> +++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
>> @@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev)
>>               return PTR_ERR(ipdev->data);
>>
>>       ipdev->heaps = devm_kzalloc(&pdev->dev,
>> -                             sizeof(struct ion_heap)*ipdev->data->nr,
>> +                             sizeof(*ipdev->heaps) * ipdev->data->nr,
>
> Do we really want to allocate an array of pointers, or an array of
> structures?  Which is it, as it's not obvious...
>
> thanks,
>
> greg k-h

Re: [Outreachy kernel] Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument

[Julia Lawall]

On Wed, 21 Sep 2016, sayli karnik wrote:

> On Wed, Sep 21, 2016 at 9:45 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote:
> >> This patch finds cases where the argument to sizeof is wrong in memory
> >> allocation functions by checking the type of the allocated memory when it is a
> >> double pointer and ensuring the sizeof argument takes a pointer to the memory
> >> being allocated.
> >
> > Are you sure?
> >
> This was detected by scripts/coccinelle/misc/badty.cocci, which
> specifies that a false positive only arises when the sizeof argument
> is not used in constructing the return value. Also the driver builds
> without errors. What do you think?

Neither of these are convincing argument.  For example, if you replaced
the sizeof by 27, it would also build.  And Coccinelle doesn't know
anything aboout the intent of the code.  So you would have to check it
carefully.

>
> >
> >> Done using coccinelle:
> >> @@
> >> type T;
> >> T **x;
> >> @@
> >>
> >>   x =
> >>   <+...sizeof(
> >> - T
> >> + *x

This change looks appropriate for a single pointer case, not a double
pointer case.  Ie where x has type T *x.  Check the code again and think
about what types are involved.

julia

> >>   )...+>
> >>
> >> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
> >> ---
> >>  drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> >> index 659aa71..9f9b244 100644
> >> --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> >> +++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> >> @@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev)
> >>               return PTR_ERR(ipdev->data);
> >>
> >>       ipdev->heaps = devm_kzalloc(&pdev->dev,
> >> -                             sizeof(struct ion_heap)*ipdev->data->nr,
> >> +                             sizeof(*ipdev->heaps) * ipdev->data->nr,
> >
> > Do we really want to allocate an array of pointers, or an array of
> > structures?  Which is it, as it's not obvious...
> >
> > thanks,
> >
> > greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAKG5xWgV-0cKboce8i8LbLB7kBQPMYs_NjcredvggXX6Qb%3DUGA%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument

[Greg Kroah-Hartman]

On Wed, Sep 21, 2016 at 10:01:09PM +0530, sayli karnik wrote:
> On Wed, Sep 21, 2016 at 9:45 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote:
> >> This patch finds cases where the argument to sizeof is wrong in memory
> >> allocation functions by checking the type of the allocated memory when it is a
> >> double pointer and ensuring the sizeof argument takes a pointer to the memory
> >> being allocated.
> >
> > Are you sure?
> >
> This was detected by scripts/coccinelle/misc/badty.cocci, which
> specifies that a false positive only arises when the sizeof argument
> is not used in constructing the return value. Also the driver builds
> without errors. What do you think?

building without errors is not the issue here at all, it's a number that
is being passed in, you could replace that statement with "42" and it
would still build.

But would it work when running?  That's the question here :)

Please look at the surrounding code and see if it is correct or not,
somehow it is working as-is, right?  Are we just allocating too much
memory?

Pointer math is tricky at times, I can understand coccinelle scripts
getting confused here...

thanks,

greg k-h