linux-kernel - Re: [PATCH v3] checkpatch: extend attributes check to handle more patterns

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 24 Oct 2020 16:21:36 -0700
From:   Joe Perches <joe@...ches.com>
To:     Dwaipayan Ray <dwaipayanray1@...il.com>
Cc:     linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org, lukas.bulwahn@...il.com
Subject: Re: [PATCH v3] checkpatch: extend attributes check to handle more
 patterns

On Sat, 2020-10-24 at 14:35 +0530, Dwaipayan Ray wrote:
> It is generally preferred that the macros from
> include/linux/compiler_attributes.h are used, unless there
> is a reason not to.
> 
> checkpatch currently checks __attribute__ for each of
> packed, aligned, printf, scanf, and weak. Other declarations
> in compiler_attributes.h are not handled.
> 
> Add a generic test to check the presence of such attributes.
> Some attributes require more specific handling and are kept
> separate.
[]
> -		}
> +		    $line =~ /__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
> +			my $attr = $1;
> +			$attr =~ s/\s*\(\s*(.*)\)\s*/$1/;
> +
> +			my %attr_list = (
> +				"alias"				=> "__alias",
> +				"aligned"			=> "__aligned",
> +				"always_inline"			=> "__always_inline",
> +				"assume_aligned"		=> "__assume_aligned",
> +				"cold"				=> "__cold",
> +				"const"				=> "__const",
> +				"copy"				=> "__copy",
> +				"designated_init"		=> "__designated_init",
> +				"externally_visible"		=> "__visible",
> +				"fallthrough"			=> "fallthrough",

I'd remove fallthrough.

It doesn't make sense as the attribute could be in any line
of a switch/case block and fallthrough; must be the last line
of the block.

> +			if ($attr =~ /^(\w+)\s*(${balanced_parens})?/) {
> +				my $curr_attr = $1;
> +				my $params = '';
> +				$params = $2 if defined($2);
> +				$curr_attr =~ s/^[\s_]+|[\s_]+$//g;
> +
> +				if (exists($attr_list{$curr_attr})) {
> +					my $new = $attr_list{$curr_attr};
> +					WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +					     "$new$params is preffered over __attribute__(($attr))\n" . $herecurr);

Be nice to have a $fix option here

> +			# Check for __attribute__ format(printf, prefer __printf
> +			if ($attr =~ /^_*format_*\s*\(\s*printf/) {
> +				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +				         "__printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n" . $herecurr) &&
> +					$fix) {
> +					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf\s*,\s*(.*)\)\s*\)\s*\)/"__printf(" . trim($1) . ")"/ex;

like for format(printf, index, pos)
and format(scanf, index, pos)