[PATCH] x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[PATCH] x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 4 Sep 2016 22:15:09 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/kernel/ksysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
index 4afc67f..cddf3c6 100644
--- a/arch/x86/kernel/ksysfs.c
+++ b/arch/x86/kernel/ksysfs.c
@@ -283,7 +283,7 @@ static int __init create_setup_data_nodes(struct kobject *parent)
 	if (ret)
 		goto out_setup_data_kobj;
 
-	kobjp = kmalloc(sizeof(*kobjp) * nr, GFP_KERNEL);
+	kobjp = kmalloc_array(nr, sizeof(*kobjp), GFP_KERNEL);
 	if (!kobjp) {
 		ret = -ENOMEM;
 		goto out_setup_data_kobj;
-- 
2.9.3

[PATCH] x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 4 Sep 2016 22:15:09 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/kernel/ksysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
index 4afc67f..cddf3c6 100644
--- a/arch/x86/kernel/ksysfs.c
+++ b/arch/x86/kernel/ksysfs.c
@@ -283,7 +283,7 @@ static int __init create_setup_data_nodes(struct kobject *parent)
 	if (ret)
 		goto out_setup_data_kobj;
 
-	kobjp = kmalloc(sizeof(*kobjp) * nr, GFP_KERNEL);
+	kobjp = kmalloc_array(nr, sizeof(*kobjp), GFP_KERNEL);
 	if (!kobjp) {
 		ret = -ENOMEM;
 		goto out_setup_data_kobj;
-- 
2.9.3

Re: [PATCH] x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Kees Cook]

On Sun, Sep 4, 2016 at 4:23 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 4 Sep 2016 22:15:09 +0200
>
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.

Which rule-set was used?

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

While probably impossible to overflow in the real world, it's still
better to use the right function here.

Acked-by: Kees Cook <keescook@chromium.org>

> ---
>  arch/x86/kernel/ksysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
> index 4afc67f..cddf3c6 100644
> --- a/arch/x86/kernel/ksysfs.c
> +++ b/arch/x86/kernel/ksysfs.c
> @@ -283,7 +283,7 @@ static int __init create_setup_data_nodes(struct kobject *parent)
>         if (ret)
>                 goto out_setup_data_kobj;
>
> -       kobjp = kmalloc(sizeof(*kobjp) * nr, GFP_KERNEL);
> +       kobjp = kmalloc_array(nr, sizeof(*kobjp), GFP_KERNEL);
>         if (!kobjp) {
>                 ret = -ENOMEM;
>                 goto out_setup_data_kobj;
> --
> 2.9.3
>



-- 
Kees Cook
Nexus Security

Re: [PATCH] x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Kees Cook]

On Sun, Sep 4, 2016 at 4:23 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 4 Sep 2016 22:15:09 +0200
>
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.

Which rule-set was used?

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

While probably impossible to overflow in the real world, it's still
better to use the right function here.

Acked-by: Kees Cook <keescook@chromium.org>

> ---
>  arch/x86/kernel/ksysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
> index 4afc67f..cddf3c6 100644
> --- a/arch/x86/kernel/ksysfs.c
> +++ b/arch/x86/kernel/ksysfs.c
> @@ -283,7 +283,7 @@ static int __init create_setup_data_nodes(struct kobject *parent)
>         if (ret)
>                 goto out_setup_data_kobj;
>
> -       kobjp = kmalloc(sizeof(*kobjp) * nr, GFP_KERNEL);
> +       kobjp = kmalloc_array(nr, sizeof(*kobjp), GFP_KERNEL);
>         if (!kobjp) {
>                 ret = -ENOMEM;
>                 goto out_setup_data_kobj;
> --
> 2.9.3
>



-- 
Kees Cook
Nexus Security

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

>> A multiplication for the size determination of a memory allocation
>> indicated that an array data structure should be processed.
>> Thus use the corresponding function "kmalloc_array".
>>
>> This issue was detected by using the Coccinelle software.
> 
> Which rule-set was used?

Do you get an useful impression from one of my bug reports
(or feature requests) like "Fix usage of white-space characters
at two places for Linux coding style" which evolved together with
the proposed software refactoring?
https://github.com/coccinelle/coccinelle/issues/76

I am curious if the time is ready now for the development of another
script for the semantic patch language which can be executed
by the make interface "coccicheck" after Julia Lawall improved
also software components a bit more recently.
https://github.com/coccinelle/coccinelle/commits/


Are you looking for further possibilities to improve the involved
source code search patterns?

Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

>> A multiplication for the size determination of a memory allocation
>> indicated that an array data structure should be processed.
>> Thus use the corresponding function "kmalloc_array".
>>
>> This issue was detected by using the Coccinelle software.
> 
> Which rule-set was used?

Do you get an useful impression from one of my bug reports
(or feature requests) like "Fix usage of white-space characters
at two places for Linux coding style" which evolved together with
the proposed software refactoring?
https://github.com/coccinelle/coccinelle/issues/76

I am curious if the time is ready now for the development of another
script for the semantic patch language which can be executed
by the make interface "coccicheck" after Julia Lawall improved
also software components a bit more recently.
https://github.com/coccinelle/coccinelle/commits/


Are you looking for further possibilities to improve the involved
source code search patterns?

Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Paolo Bonzini]

On 07/09/2016 09:49, SF Markus Elfring wrote:
>>> A multiplication for the size determination of a memory allocation
>>> indicated that an array data structure should be processed.
>>> Thus use the corresponding function "kmalloc_array".
>>>
>>> This issue was detected by using the Coccinelle software.
>>
>> Which rule-set was used?
> 
> Do you get an useful impression from one of my bug reports
> (or feature requests) like "Fix usage of white-space characters
> at two places for Linux coding style" which evolved together with
> the proposed software refactoring?
> https://github.com/coccinelle/coccinelle/issues/76
> 
> I am curious if the time is ready now for the development of another
> script for the semantic patch language which can be executed
> by the make interface "coccicheck" after Julia Lawall improved
> also software components a bit more recently.
> https://github.com/coccinelle/coccinelle/commits/
> 
> Are you looking for further possibilities to improve the involved
> source code search patterns?

Why are you not answering the simple question that was asked?

Paolo

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Paolo Bonzini]

On 07/09/2016 09:49, SF Markus Elfring wrote:
>>> A multiplication for the size determination of a memory allocation
>>> indicated that an array data structure should be processed.
>>> Thus use the corresponding function "kmalloc_array".
>>>
>>> This issue was detected by using the Coccinelle software.
>>
>> Which rule-set was used?
> 
> Do you get an useful impression from one of my bug reports
> (or feature requests) like "Fix usage of white-space characters
> at two places for Linux coding style" which evolved together with
> the proposed software refactoring?
> https://github.com/coccinelle/coccinelle/issues/76
> 
> I am curious if the time is ready now for the development of another
> script for the semantic patch language which can be executed
> by the make interface "coccicheck" after Julia Lawall improved
> also software components a bit more recently.
> https://github.com/coccinelle/coccinelle/commits/
> 
> Are you looking for further possibilities to improve the involved
> source code search patterns?

Why are you not answering the simple question that was asked?

Paolo

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

>> Are you looking for further possibilities to improve the involved
>> source code search patterns?
> 
> Why are you not answering the simple question that was asked?

I find that I answered it to some degree.

It can be that you do not really like the kind of answer that I chose
a moment ago.
But I guess that a more pleasing (and complete) answer can become
another software development challenge as you might know already.

Would the following script (for the semantic patch language)
be useful enough for further development considerations?

usage_of_kmalloc_array1-excerpt2.cocci:
@replacement2@
expression count, pointer, target;
@@
 target =
-         kmalloc(sizeof(*pointer) * (count)
+         kmalloc_array(count, sizeof(*pointer)
                        , ...);

Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

>> Are you looking for further possibilities to improve the involved
>> source code search patterns?
> 
> Why are you not answering the simple question that was asked?

I find that I answered it to some degree.

It can be that you do not really like the kind of answer that I chose
a moment ago.
But I guess that a more pleasing (and complete) answer can become
another software development challenge as you might know already.

Would the following script (for the semantic patch language)
be useful enough for further development considerations?

usage_of_kmalloc_array1-excerpt2.cocci:
@replacement2@
expression count, pointer, target;
@@
 target -         kmalloc(sizeof(*pointer) * (count)
+         kmalloc_array(count, sizeof(*pointer)
                        , ...);

Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Paolo Bonzini]

On 07/09/2016 13:17, SF Markus Elfring wrote:
>>> Are you looking for further possibilities to improve the involved
>>> source code search patterns?
>>
>> Why are you not answering the simple question that was asked?
> 
> I find that I answered it to some degree.
> 
> It can be that you do not really like the kind of answer that I chose
> a moment ago.
> But I guess that a more pleasing (and complete) answer can become
> another software development challenge as you might know already.
> 
> Would the following script (for the semantic patch language)
> be useful enough for further development considerations?
> 
> usage_of_kmalloc_array1-excerpt2.cocci:
> @replacement2@
> expression count, pointer, target;
> @@
>  target =
> -         kmalloc(sizeof(*pointer) * (count)
> +         kmalloc_array(count, sizeof(*pointer)
>                         , ...);

Why don't you include the _exact_ script that you run?  That's the only
possible correct answer.

Paolo

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Paolo Bonzini]

On 07/09/2016 13:17, SF Markus Elfring wrote:
>>> Are you looking for further possibilities to improve the involved
>>> source code search patterns?
>>
>> Why are you not answering the simple question that was asked?
> 
> I find that I answered it to some degree.
> 
> It can be that you do not really like the kind of answer that I chose
> a moment ago.
> But I guess that a more pleasing (and complete) answer can become
> another software development challenge as you might know already.
> 
> Would the following script (for the semantic patch language)
> be useful enough for further development considerations?
> 
> usage_of_kmalloc_array1-excerpt2.cocci:
> @replacement2@
> expression count, pointer, target;
> @@
>  target > -         kmalloc(sizeof(*pointer) * (count)
> +         kmalloc_array(count, sizeof(*pointer)
>                         , ...);

Why don't you include the _exact_ script that you run?  That's the only
possible correct answer.

Paolo

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

>> Would the following script (for the semantic patch language)
>> be useful enough for further development considerations?
>>
>> usage_of_kmalloc_array1-excerpt2.cocci:
>> @replacement2@
>> expression count, pointer, target;
>> @@
>>  target =
>> -         kmalloc(sizeof(*pointer) * (count)
>> +         kmalloc_array(count, sizeof(*pointer)
>>                         , ...);
> 
> Why don't you include the _exact_ script that you run?

I showed only the "excerpt" above because of the current situation
that this single SmPL rule triggered the software change
which I suggested for the referenced source file.

How do you think about to try a command out like the following
also in your development (or test) environment?

elfring@Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/usage_of_kmalloc_array1-excerpt2.cocci arch/x86/kernel/ksysfs.c


Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

>> Would the following script (for the semantic patch language)
>> be useful enough for further development considerations?
>>
>> usage_of_kmalloc_array1-excerpt2.cocci:
>> @replacement2@
>> expression count, pointer, target;
>> @@
>>  target >> -         kmalloc(sizeof(*pointer) * (count)
>> +         kmalloc_array(count, sizeof(*pointer)
>>                         , ...);
> 
> Why don't you include the _exact_ script that you run?

I showed only the "excerpt" above because of the current situation
that this single SmPL rule triggered the software change
which I suggested for the referenced source file.

How do you think about to try a command out like the following
also in your development (or test) environment?

elfring@Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/usage_of_kmalloc_array1-excerpt2.cocci arch/x86/kernel/ksysfs.c


Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Kees Cook]

On Wed, Sep 7, 2016 at 4:45 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> Would the following script (for the semantic patch language)
>>> be useful enough for further development considerations?
>>>
>>> usage_of_kmalloc_array1-excerpt2.cocci:
>>> @replacement2@
>>> expression count, pointer, target;
>>> @@
>>>  target =
>>> -         kmalloc(sizeof(*pointer) * (count)
>>> +         kmalloc_array(count, sizeof(*pointer)
>>>                         , ...);
>>
>> Why don't you include the _exact_ script that you run?
>
> I showed only the "excerpt" above because of the current situation
> that this single SmPL rule triggered the software change
> which I suggested for the referenced source file.
>
> How do you think about to try a command out like the following
> also in your development (or test) environment?
>
> elfring@Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/usage_of_kmalloc_array1-excerpt2.cocci arch/x86/kernel/ksysfs.c

Fixing these kmalloc calls would be a nice thing to clean up
everywhere. Since it is a mistake people may continue to make, I think
it would make sense to add a coccinelle script that can do this to the
existing coccinelle scripts in the kernel if one to do it does not
already exist. That way, it will be part of the coccinelle checking
that is automatically run on the kernel regularly.

-Kees

-- 
Kees Cook
Nexus Security

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Kees Cook]

On Wed, Sep 7, 2016 at 4:45 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> Would the following script (for the semantic patch language)
>>> be useful enough for further development considerations?
>>>
>>> usage_of_kmalloc_array1-excerpt2.cocci:
>>> @replacement2@
>>> expression count, pointer, target;
>>> @@
>>>  target >>> -         kmalloc(sizeof(*pointer) * (count)
>>> +         kmalloc_array(count, sizeof(*pointer)
>>>                         , ...);
>>
>> Why don't you include the _exact_ script that you run?
>
> I showed only the "excerpt" above because of the current situation
> that this single SmPL rule triggered the software change
> which I suggested for the referenced source file.
>
> How do you think about to try a command out like the following
> also in your development (or test) environment?
>
> elfring@Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/usage_of_kmalloc_array1-excerpt2.cocci arch/x86/kernel/ksysfs.c

Fixing these kmalloc calls would be a nice thing to clean up
everywhere. Since it is a mistake people may continue to make, I think
it would make sense to add a coccinelle script that can do this to the
existing coccinelle scripts in the kernel if one to do it does not
already exist. That way, it will be part of the coccinelle checking
that is automatically run on the kernel regularly.

-Kees

-- 
Kees Cook
Nexus Security

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Joe Perches]

On Wed, 2016-09-07 at 09:23 -0700, Kees Cook wrote:
> Fixing these kmalloc calls would be a nice thing to clean up
> everywhere.

Dubious as gcc cannot currently optimize known small fixed size
allocations with alloc_array and will always perform the
multiplication.

Also the style of sizeof(*ptr) is not always as clear, obvious
nor as easy to grep as sizeof(type).

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Joe Perches]

On Wed, 2016-09-07 at 09:23 -0700, Kees Cook wrote:
> Fixing these kmalloc calls would be a nice thing to clean up
> everywhere.

Dubious as gcc cannot currently optimize known small fixed size
allocations with alloc_array and will always perform the
multiplication.

Also the style of sizeof(*ptr) is not always as clear, obvious
nor as easy to grep as sizeof(type).

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

> Fixing these kmalloc calls would be a nice thing to clean up everywhere.

Thanks for your acknowledgement of such a software improvement opportunity.


> Since it is a mistake people may continue to make, I think it would
> make sense to add a coccinelle script that can do this to the
> existing coccinelle scripts in the kernel if one to do it does not
> already exist. That way, it will be part of the coccinelle checking
> that is automatically run on the kernel regularly.

I am curious on how many contributors would like to help a bit more
in such a software development task.

How do you think about to clarify corresponding challenges better?

Do you find any details interesting which are described in a feature
request (or article) like "Improve determination of sizes with SmPL"?
https://github.com/coccinelle/coccinelle/issues/80

Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[SF Markus Elfring]

> Fixing these kmalloc calls would be a nice thing to clean up everywhere.

Thanks for your acknowledgement of such a software improvement opportunity.


> Since it is a mistake people may continue to make, I think it would
> make sense to add a coccinelle script that can do this to the
> existing coccinelle scripts in the kernel if one to do it does not
> already exist. That way, it will be part of the coccinelle checking
> that is automatically run on the kernel regularly.

I am curious on how many contributors would like to help a bit more
in such a software development task.

How do you think about to clarify corresponding challenges better?

Do you find any details interesting which are described in a feature
request (or article) like "Improve determination of sizes with SmPL"?
https://github.com/coccinelle/coccinelle/issues/80

Regards,
Markus

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Kees Cook]

On Wed, Sep 7, 2016 at 9:37 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2016-09-07 at 09:23 -0700, Kees Cook wrote:
>> Fixing these kmalloc calls would be a nice thing to clean up
>> everywhere.
>
> Dubious as gcc cannot currently optimize known small fixed size
> allocations with alloc_array and will always perform the
> multiplication.

Oh, that's sad. Is it not able to tell what the types are to basic
overflow validation? Could we help gcc in some way?

> Also the style of sizeof(*ptr) is not always as clear, obvious
> nor as easy to grep as sizeof(type).

I've always been on the fence about this (I like being able to see
destination:thing, sizeof(*thing) for easy scanning), but having the
type right there can be easier for text searches.

-Kees

-- 
Kees Cook
Nexus Security

Re: x86-ksysfs: Use kmalloc_array() in create_setup_data_nodes()

[Kees Cook]

On Wed, Sep 7, 2016 at 9:37 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2016-09-07 at 09:23 -0700, Kees Cook wrote:
>> Fixing these kmalloc calls would be a nice thing to clean up
>> everywhere.
>
> Dubious as gcc cannot currently optimize known small fixed size
> allocations with alloc_array and will always perform the
> multiplication.

Oh, that's sad. Is it not able to tell what the types are to basic
overflow validation? Could we help gcc in some way?

> Also the style of sizeof(*ptr) is not always as clear, obvious
> nor as easy to grep as sizeof(type).

I've always been on the fence about this (I like being able to see
destination:thing, sizeof(*thing) for easy scanning), but having the
type right there can be easier for text searches.

-Kees

-- 
Kees Cook
Nexus Security