Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Lukas Bulwahn]

To your questions, here is my opinion...

1. Is the header format in the semantic patch acceptable (i.e. referencing the CWE that this particular semantic patch aims to address)?

Actually, I think we should that for the existing rules as well.

I was thinking of the following format:

# Addresses: CWE-414 ("Missing Lock Check")

or

# Contributes-to: Missing Lock Check [CWE-414]

I think it is good discussion to have with Julia Lawall, Dan Carpenter, Luc Van Oostenryck, Joe Perches, etc. to see how they would want to maintain such information within their tools.


2. Should we create a separate directory for ELISA within coccinelle?

No, we do not structure according to the contributor, then the kernel architecture would be "linus directory", "andrew directory", "shuah directory", etc.

I would suggest that we could roughly structure according the existing structure for coccinelle and the CWE structure.


Lukas

P.S.: We need to set groups.io not to generate HTML emails on responses etc. when we want to engage with the kernel community. Let us check if we get that set up.
 

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Mohammed Billoo]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

With regards to the directory name, it wasn't meant to be for this specific
group, but a sensible category to put these patches in. In my experience,
misc becomes a catch-all for everything else, promotes laziness, and things
end up being a mess. Does it make sense to create another directory that
contains patches that are specific to catching CWEs (e.g. a directory
called "safety" or "safety-critical") that don't obviously fall into the
other non-misc directories?

On Thu, Aug 13, 2020 at 11:16 AM <Lukas.Bulwahn@bmw.de> wrote:

> To your questions, here is my opinion...
>
> 1. Is the header format in the semantic patch acceptable (i.e. referencing
> the CWE that this particular semantic patch aims to address)?
>
> Actually, I think we should that for the existing rules as well.
>
> I was thinking of the following format:
>
> # Addresses: CWE-414 ("Missing Lock Check")
>
> or
>
> # Contributes-to: Missing Lock Check [CWE-414]
>
> I think it is good discussion to have with Julia Lawall, Dan Carpenter,
> Luc Van Oostenryck, Joe Perches, etc. to see how they would want to
> maintain such information within their tools.
>
>
> 2. Should we create a separate directory for ELISA within coccinelle?
>
> No, we do not structure according to the contributor, then the kernel
> architecture would be "linus directory", "andrew directory", "shuah
> directory", etc.
>
> I would suggest that we could roughly structure according the existing
> structure for coccinelle and the CWE structure.
>
>
> Lukas
>
> P.S.: We need to set groups.io not to generate HTML emails on responses
> etc. when we want to engage with the kernel community. Let us check if we
> get that set up.
>
>

-- 
Mohammed A Billoo
Founder
MAB Labs, LLC
www.mab-labs.com
201-338-2022

[-- Attachment #2: Type: text/html, Size: 2471 bytes --]

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Lukas Bulwahn]

[-- Attachment #1: Type: text/plain, Size: 2512 bytes --]

Sorry for top posting.

No, safety is CERTAINLY NOT a category. Security maybe, but even better would be a category like “information leaks” and a subset “kernel-internal information leaks” for your specific coccinelle rule addressing CWE-547.

For me, for now, misc is okay, but if we want to restructure and clean-up, we should come up with a complete picture that fits for all.

Lukas

Von: Mohammed Billoo <mab@mab-labs.com>
Gesendet: Donnerstag, 13. August 2020 17:24
An: Bulwahn Lukas, JC-20 <Lukas.Bulwahn@bmw.de>
Cc: Shuah Khan <skhan@linuxfoundation.org>; linux-safety@lists.elisa.tech
Betreff: Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

With regards to the directory name, it wasn't meant to be for this specific group, but a sensible category to put these patches in. In my experience, misc becomes a catch-all for everything else, promotes laziness, and things end up being a mess. Does it make sense to create another directory that contains patches that are specific to catching CWEs (e.g. a directory called "safety" or "safety-critical") that don't obviously fall into the other non-misc directories?

On Thu, Aug 13, 2020 at 11:16 AM <Lukas.Bulwahn@bmw.de<mailto:Lukas.Bulwahn@bmw.de>> wrote:
To your questions, here is my opinion...

1. Is the header format in the semantic patch acceptable (i.e. referencing the CWE that this particular semantic patch aims to address)?

Actually, I think we should that for the existing rules as well.

I was thinking of the following format:

# Addresses: CWE-414 ("Missing Lock Check")

or

# Contributes-to: Missing Lock Check [CWE-414]

I think it is good discussion to have with Julia Lawall, Dan Carpenter, Luc Van Oostenryck, Joe Perches, etc. to see how they would want to maintain such information within their tools.


2. Should we create a separate directory for ELISA within coccinelle?

No, we do not structure according to the contributor, then the kernel architecture would be "linus directory", "andrew directory", "shuah directory", etc.

I would suggest that we could roughly structure according the existing structure for coccinelle and the CWE structure.


Lukas

P.S.: We need to set groups.io<http://groups.io> not to generate HTML emails on responses etc. when we want to engage with the kernel community. Let us check if we get that set up.


--
Mohammed A Billoo
Founder
MAB Labs, LLC
www.mab-labs.com<http://www.mab-labs.com>
201-338-2022

[-- Attachment #2: Type: text/html, Size: 6133 bytes --]

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Sudip Mukherjee]

On 13/08/2020 16:33, Lukas Bulwahn wrote:
> Sorry for top posting.
> 
>  
> 
> No, safety is CERTAINLY NOT a category. Security maybe, but even better
> would be a category like “information leaks” and a subset
> “kernel-internal information leaks” for your specific coccinelle rule
> addressing CWE-547.
> 
>  
> 
> For me, for now, misc is okay, but if we want to restructure and
> clean-up, we should come up with a complete picture that fits for all.

imho, misc is ok for this one, but when you actually make a cocci script
for CWE-414 ("Missing Lock Check"), that should be going to
scripts/coccinelle/locks/



-- 
Regards
Sudip

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Shuah Khan]

On 8/13/20 9:41 AM, Sudip Mukherjee wrote:
> 
> 
> On 13/08/2020 16:33, Lukas Bulwahn wrote:
>> Sorry for top posting.
>>
>>   
>>
>> No, safety is CERTAINLY NOT a category. Security maybe, but even better
>> would be a category like “information leaks” and a subset
>> “kernel-internal information leaks” for your specific coccinelle rule
>> addressing CWE-547.
>>

+1

>>   
>>
>> For me, for now, misc is okay, but if we want to restructure and
>> clean-up, we should come up with a complete picture that fits for all.
> 
> imho, misc is ok for this one, but when you actually make a cocci script
> for CWE-414 ("Missing Lock Check"), that should be going to
> scripts/coccinelle/locks/
> 

+1
Agree with Lukas and Sudip on directory - safety isn't appropriate here.

You can find a suitable place: current coverage areas under
scripts/coccinelle are

api  free  iterators  locks  misc  null  tests

Let's try to map new scripts to these categories or create a new
category when one doesn't exist.

thanks,
-- Shuah

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Shuah Khan]

On 8/13/20 11:02 AM, Shuah Khan via lists.elisa.tech wrote:
> On 8/13/20 9:41 AM, Sudip Mukherjee wrote:
>>
>>
>> On 13/08/2020 16:33, Lukas Bulwahn wrote:
>>> Sorry for top posting.
>>>
>>>
>>> No, safety is CERTAINLY NOT a category. Security maybe, but even better
>>> would be a category like “information leaks” and a subset
>>> “kernel-internal information leaks” for your specific coccinelle rule
>>> addressing CWE-547.
>>>
> 
> +1
> 
>>>
>>> For me, for now, misc is okay, but if we want to restructure and
>>> clean-up, we should come up with a complete picture that fits for all.
>>
>> imho, misc is ok for this one, but when you actually make a cocci script
>> for CWE-414 ("Missing Lock Check"), that should be going to
>> scripts/coccinelle/locks/
>>
> 
> +1
> Agree with Lukas and Sudip on directory - safety isn't appropriate here.
> 
> You can find a suitable place: current coverage areas under
> scripts/coccinelle are
> 
> api  free  iterators  locks  misc  null  tests
> 
> Let's try to map new scripts to these categories or create a new
> category when one doesn't exist.
> 

One more thing. Also look into if these issues can be found by compiler.
If so, is there a need to come up with coccinelle script.

Somehow this sounds like a basic error compiler should be able to flag
and might already have a method in the kernel.

thanks,
-- Shuah

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Lukas Bulwahn]

> imho, misc is ok for this one, but when you actually make a cocci script for
> CWE-414 ("Missing Lock Check"), that should be going to
> scripts/coccinelle/locks/
> 

Agree. Locking checks go to locks.

The example of CWE-414 was just to show a suitable format for adding the CWE mapping into the tools/rules as comments and maintain the mapping there.


Lukas

[PATCH] coccinelle: misc: Check for hard-coded constants

[Mohammed Billoo]

This semantic patch looks for variables that are initialized to
constants, arrays that are both declared and indexed with constants.
A false positive will occur  when a variable is initialized to 0, which
must happen for auto variables. This will be resolved in a future patch.

The patch was tested against the following snippet:

int main()
{
    int iarr[54]; /* instance 1 */
    int j = 0;    /* instance 2 */
    int i = 1;    /* instance 3 */
    iarr[0] = 3;  /* instance 4 */
    return 0;
}

and it correctly identified instances 1, 3, and 4. It incorrectly
identified instance 2, which will be addressed in a future patch.

Signed-off-by: Mohammed Billoo <mab@mab-labs.com>
---
 scripts/coccinelle/misc/magic_numbers.cocci | 45 +++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 scripts/coccinelle/misc/magic_numbers.cocci

diff --git a/scripts/coccinelle/misc/magic_numbers.cocci b/scripts/coccinelle/misc/magic_numbers.cocci
new file mode 100644
index 000000000000..be6df33d28e4
--- /dev/null
+++ b/scripts/coccinelle/misc/magic_numbers.cocci
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Capture and instances of CWE-547 (https://cwe.mitre.org/data/definitions/547.html)
+///
+//# This attempts to capture instances of magic numbers and report them
+
+virtual report
+
+@r1 depends on report@
+type T;
+constant C;
+identifier var;
+position p;
+@@
+* T var@p = C;
+
+@script:python depends on report@
+p << r1.p;
+@@
+coccilib.report.print_report(p[0], "Hard-coded constant, consider using #define")
+
+@r2 depends on report@
+type T;
+identifier var;
+constant C;
+position p;
+@@
+* T var[C];
+
+@script:python depends on report@
+p << r2.p;
+@@
+coccilib.report.print_report(p[0], "Hard-coded constant, consider using #define")
+
+@r3 depends on report@
+type T;
+constant C;
+position p;
+T[] E;
+@@
+* E[C]@p = ... ;
+
+@script:python depends on report@
+p << r3.p;
+@@
+coccilib.report.print_report(p[0], "Hard-coded constant, consider using #define")
-- 
2.17.1

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Shuah Khan]

Hi Mohammed,

Thanks for your patch.

On 8/12/20 5:43 PM, Mohammed Billoo wrote:
> This semantic patch looks for variables that are initialized to
> constants, arrays that are both declared and indexed with constants.
> A false positive will occur  when a variable is initialized to 0, which
> must happen for auto variables. This will be resolved in a future patch.
> 
> The patch was tested against the following snippet:
> 
> int main()
> {
>      int iarr[54]; /* instance 1 */
>      int j = 0;    /* instance 2 */
>      int i = 1;    /* instance 3 */
>      iarr[0] = 3;  /* instance 4 */
>      return 0;
> }
> 
> and it correctly identified instances 1, 3, and 4. It incorrectly
> identified instance 2, which will be addressed in a future patch.

Please include the output from the tool that corresponds to your
changes to the script in the commit log on a kernel file.

Also I see 3 patches with incremental changes to the script. Please
make this a patch series which will make it easier for reviewers.

thanks,
-- Shuah

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Mohammed Billoo]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]

Shuah,

Apologies for the spam. I didn't format the initial correctly and needed
two more tries to get it right (according to the kernel
standard/best-practice). I can resubmit this patch.

Thanks

On Thu, Aug 13, 2020 at 10:39 AM Shuah Khan <skhan@linuxfoundation.org>
wrote:

> Hi Mohammed,
>
> Thanks for your patch.
>
> On 8/12/20 5:43 PM, Mohammed Billoo wrote:
> > This semantic patch looks for variables that are initialized to
> > constants, arrays that are both declared and indexed with constants.
> > A false positive will occur  when a variable is initialized to 0, which
> > must happen for auto variables. This will be resolved in a future patch.
> >
> > The patch was tested against the following snippet:
> >
> > int main()
> > {
> >      int iarr[54]; /* instance 1 */
> >      int j = 0;    /* instance 2 */
> >      int i = 1;    /* instance 3 */
> >      iarr[0] = 3;  /* instance 4 */
> >      return 0;
> > }
> >
> > and it correctly identified instances 1, 3, and 4. It incorrectly
> > identified instance 2, which will be addressed in a future patch.
>
> Please include the output from the tool that corresponds to your
> changes to the script in the commit log on a kernel file.
>
> Also I see 3 patches with incremental changes to the script. Please
> make this a patch series which will make it easier for reviewers.
>
> thanks,
> -- Shuah
>


-- 
Mohammed A Billoo
Founder
MAB Labs, LLC
www.mab-labs.com
201-338-2022

[-- Attachment #2: Type: text/html, Size: 2170 bytes --]

Re: [linux-safety] [PATCH] coccinelle: misc: Check for hard-coded constants

[Mohammed Billoo]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

I also had a few more questions regarding the overall format:
1. Is the header format in the semantic patch acceptable (i.e. referencing
the CWE that this particular semantic patch aims to address)?
2. Should we create a separate directory for ELISA within coccinelle?

Thanks
Mohammed

On Thu, Aug 13, 2020 at 10:42 AM Mohammed Billoo via lists.elisa.tech
<mab=mab-labs.com@lists.elisa.tech> wrote:

> Shuah,
>
> Apologies for the spam. I didn't format the initial correctly and needed
> two more tries to get it right (according to the kernel
> standard/best-practice). I can resubmit this patch.
>
> Thanks
>
> On Thu, Aug 13, 2020 at 10:39 AM Shuah Khan <skhan@linuxfoundation.org>
> wrote:
>
>> Hi Mohammed,
>>
>> Thanks for your patch.
>>
>> On 8/12/20 5:43 PM, Mohammed Billoo wrote:
>> > This semantic patch looks for variables that are initialized to
>> > constants, arrays that are both declared and indexed with constants.
>> > A false positive will occur  when a variable is initialized to 0, which
>> > must happen for auto variables. This will be resolved in a future patch.
>> >
>> > The patch was tested against the following snippet:
>> >
>> > int main()
>> > {
>> >      int iarr[54]; /* instance 1 */
>> >      int j = 0;    /* instance 2 */
>> >      int i = 1;    /* instance 3 */
>> >      iarr[0] = 3;  /* instance 4 */
>> >      return 0;
>> > }
>> >
>> > and it correctly identified instances 1, 3, and 4. It incorrectly
>> > identified instance 2, which will be addressed in a future patch.
>>
>> Please include the output from the tool that corresponds to your
>> changes to the script in the commit log on a kernel file.
>>
>> Also I see 3 patches with incremental changes to the script. Please
>> make this a patch series which will make it easier for reviewers.
>>
>> thanks,
>> -- Shuah
>>
>
>
> --
> Mohammed A Billoo
> Founder
> MAB Labs, LLC
> www.mab-labs.com
> 201-338-2022
> 
>
>

-- 
Mohammed A Billoo
Founder
MAB Labs, LLC
www.mab-labs.com
201-338-2022

[-- Attachment #2: Type: text/html, Size: 3137 bytes --]