linux-kernel - Re: [PATCH] checkpatch: Check for .byte-spelled insn opcodes documentation on x86

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]

Message-ID: <b57a59bc80e432c7696b347a223eb12339013970.camel@perches.com>
Date:   Fri, 09 Oct 2020 11:01:18 -0700
From:   Joe Perches <joe@...ches.com>
To:     Borislav Petkov <bp@...en8.de>, X86 ML <x86@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Andy Whitcroft <apw@...onical.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] checkpatch: Check for .byte-spelled insn opcodes
 documentation on x86

On Fri, 2020-10-09 at 18:14 +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@...e.de>
> 
> Instruction opcode bytes spelled using the gas directive .byte should
> carry a comment above them stating which binutils version has added
> support for the instruction mnemonic so that they can be replaced with
> the mnemonic when that binutils version is equal or less than the
> minimum-supported version by the kernel.
> 
> Add a check for that.

OK but several notes:

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -6858,6 +6858,18 @@ sub process {
>  			WARN("DUPLICATED_SYSCTL_CONST",
>  				"duplicated sysctl range checking value '$1', consider using the shared one in include/linux/sysctl.h\n" . $herecurr);
>  		}
> +
> +# document which binutils version supports the actual insn mnemonic so that the naked opcode bytes can be replaced.
> +# x86-only.
> +		if ($rawline =~ /(\.byte(?:0[xX][0-9a-fA-F]+0-9a-fx,]+)/ && $realfile =~ "^arch/x86/") {

Given the location, this only works on .c and .h files.
It does not work on .S files.  Should it?

No need for a capture group.

Please use @ not " as all the other $realfile comparisons
use that form when expecting a /

So it looks like the regex would be more complete as:

	if ($realfile =~ m@...ch/x86/@ &&
	    $rawline =~ /\.byte\s+(?:$Constant|(?:\\)?$Ident|"\s*$Ident)\b/) {

etc...

> +			my $comment = ctx_locate_comment(0, $linenr);

A patch can modify any number of files.

This should use ctx_locate_comment($file ? 0 : $first_line, $linenr)
as checkpatch tests work on patch contexts not the entire
file before this line.

> +			if (! $comment || ($comment !~ /binutils version [0-9.]+/(ms)) {

No need for the $!comment test

> +				WARN("MISSING_BINUTILS_VERSION",
> +				     "Please document which binutils version supports these .byte-spelled\n" .
> +				     "\tinsn opcodes by adding \"binutils version <num>\" in a comment" .
> +				     " above them.\n" . $herecurr);

checkpatch uses only a single line output only before $herecurr
Output line length doesn't matter.