Re: [linux-safety] [PATCH] scripts: Report 'suspicious' comments

On Thu, 27 Aug 2020, Mohammed Billoo wrote:

> This perl script attempts to mitigate CWE-546
> (https://cwe.mitre.org/data/definitions/546.html), which identifies code
> with comments that suggest that code is incomplete. This script was
> tested against the kernel, and the following is a snippet of the
> output that was generated. The output was verified by confirming that
> the specified file does indeed have that string at the specified line.
> 
> ./arch/arm/include/asm/pgtable.h contains FIXME on line 316
> ./arch/arm/include/debug/imx.S contains FIXME on line 14
> ./arch/arm/kernel/entry-header.S contains BUG on line 71
> ./arch/arm/kernel/fiq.c contains FIXME on line 72
>

Okay, easy things first:

The patch applies on v5.9-rc4 and the script runs. :)

Now, the challenging parts:

- I think your output should follow in its format the format of other 
"standard" tools that other tools use, e.g., look how gcc and clang 
compiler emit warnings, or how checkpatch.pl emits warnings.

We should try to imitate their output format for this tool.

Also, I would like to see the line which was warned about.

- Second, we then need to look into true and false positives here.
BUG appears in intended error messages, in DEBUG, in functions, such as 
BUG, BUG_ON. So we need to collect the context around BUG and see to 
improve the pattern to at least reduce the false positives.

- Third, we then need have suitable ways to aggregate the findings to know 
which places are known to have many such findings and hence deserve some 
kind of special care.

And then, we need a lot of help to make proper evaluations what to do.

Lukas 
 
> Signed-off-by: Mohammed Billoo <[email protected]>
> ---
>  Makefile                |  8 +++++++-
>  scripts/checkcomment.pl | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/checkcomment.pl
> 
> diff --git a/Makefile b/Makefile
> index f21168154160..c84b8bc5c18e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -264,7 +264,7 @@ no-dot-config-targets := $(clean-targets) \
>  			 cscope gtags TAGS tags help% %docs check% coccicheck \
>  			 $(version_h) headers headers_% archheaders archscripts \
>  			 %asm-generic kernelversion %src-pkg dt_binding_check \
> -			 outputmakefile
> +			 outputmakefile commentcheck
>  no-sync-config-targets := $(no-dot-config-targets) %install kernelrelease
>  single-targets := %.a %.i %.ko %.lds %.ll %.lst %.mod %.o %.s %.symtypes %/
>  
> @@ -1575,6 +1575,7 @@ help:
>  	@echo  '  export_report   - List the usages of all exported symbols'
>  	@echo  '  headerdep       - Detect inclusion cycles in headers'
>  	@echo  '  coccicheck      - Check with Coccinelle'
> +	@echo  '  commentcheck    - Check and report suspicious comments'
>  	@echo  ''
>  	@echo  'Tools:'
>  	@echo  '  nsdeps          - Generate missing symbol namespace dependencies'
> @@ -1842,6 +1843,11 @@ versioncheck:
>  		-name '*.[hcS]' -type f -print | sort \
>  		| xargs $(PERL) -w $(srctree)/scripts/checkversion.pl
>  
> +commentcheck:
> +	find $(srctree)/* $(RCS_FIND_IGNORE) \
> +		-name '*.[hcS]' -type f -print | sort \
> +		| xargs $(PERL) -w $(srctree)/scripts/checkcomment.pl
> +
>  coccicheck:
>  	$(Q)$(BASH) $(srctree)/scripts/$@
>  
> diff --git a/scripts/checkcomment.pl b/scripts/checkcomment.pl
> new file mode 100644
> index 000000000000..22fd77bc75d1
> --- /dev/null
> +++ b/scripts/checkcomment.pl
> @@ -0,0 +1,35 @@
> +#!/usr/bin/env perl
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# (c) 2020, Mohammed Billoo ([email protected])
> +#
> +# This script checks for any keywords outlined in CWE-546
> +# (https://cwe.mitre.org/data/definitions/546.html)
> +# and simply reports them to the user. It's up to the user
> +# to take any further actions.
> +
> +use strict;
> +
> +my @keywords = ('TODO', 'BUG', 'FIXME', 'HACK');
> +my @mismatch_keywords = ('BUG\(\)');
> +
> +foreach my $file (@ARGV) {
> +	my $i = 1;
> +	open(my $f, '<', $file)
> +		or die "Cannot open $file: $!\n";
> +
> +	while (my $line = <$f>) {
> +		foreach my $keyword (@keywords) {
> +			if ($line =~ /\b$keyword\b/) {
> +				foreach my $mismatch_keyword (@mismatch_keywords) {
> +					if ($line =~ /$mismatch_keyword/) {}
> +					else {
> +						print "$file contains $keyword on line $i\n";
> +					}
> +				}
> +			}
> +		}
> +
> +		$i++;
> +	}
> +}
> -- 
> 2.17.1
> 
> 
> 
> 
>