linux-kernel - Re: General kernel misuse of vsnprintf SPECIAL %#<foo> (was: Re: [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings)

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: <20210603202546.0d12e7ad@jic23-huawei>
Date:   Thu, 3 Jun 2021 20:25:46 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Joe Perches <joe@...ches.com>
Cc:     linux-iio@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: General kernel misuse of vsnprintf SPECIAL %#<foo> (was: Re:
 [PATCH v2 0/4] iio: Drop use of %hhx and %hx format strings)

On Thu, 03 Jun 2021 11:58:15 -0700
Joe Perches <joe@...ches.com> wrote:

> On Thu, 2021-06-03 at 19:06 +0100, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > 
> > A wrong use of one of these in
> > https://lore.kernel.org/linux-iio/20210514135927.2926482-1-arnd@kernel.org/
> > included a reference from Nathan to a patch discouraging the use of
> > these strings in general:
> > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> > 
> > I did a quick grep and established we only have a few instances of these in
> > IIO anyway, so in the interests of avoiding those existing cases getting
> > cut and paste into new drivers, let's just clear them out now.
> > 
> > Note that patch from Arnd is now also part of this series, due to the
> > length specifier related issue Joe raised.
> > 
> > I have chosen to go with 0x%02x rather than %#04x as I find it more readable.
> > 
> > V2:
> > Use 0x%02x (Joe Perches)
> > Include Arnd's original patch, modified for the above.  
> 
> Hello again.
> 
> It looks to me as though %#<foo> is relatively commonly misused in the kernel.
> 
> Pehaps for the decimal portion of the format, checkpatch could have some
> test for use of non-standard lengths.
> 
> Given the use is generally meant for a u8, u16, u32, or u64, perhaps
> checkpatch should emit a warning whenever the length is not 4, 6, 10, or 18.

Would have saved me some trouble, so I'm definitely in favour of checkpatch
catching this.

I wonder if a better option is to match on 1, 2, 4, 8, 16 as likely to be
caused by people getting the usage wrong rather than a deliberate attempt
to pretty print something a little unusual?

Thanks.

Jonathan

> 
> (possible checkpatch patch below)
> 
> $ git grep -P -h -o '%#\d+\w+' | sort | uniq -c | sort -rn
>     392 %#08x
>     238 %#04x
>     144 %#02x
>     114 %#06x
>      92 %#010x
>      58 %#010Lx
>      55 %#018llx
>      47 %#010llx
>      45 %#010lx
>      38 %#016llx
>      27 %#0x
>      23 %#2x
>      18 %#016lx
>      17 %#3lx
>      17 %#08lx
>      17 %#018Lx
>      15 %#3x
>      14 %#03x
>      10 %#06hx
>       9 %#08zx
>       8 %#10x
>       7 %#16llx
>       6 %#8x
>       6 %#04X
>       6 %#04llx
>       6 %#012llx
>       5 %#16
>       4 %#08llx
>       4 %#06llx
>       4 %#05x
>       4 %#02X
>       4 %#016Lx
>       3 %#04hx
>       3 %#01x
>       2 %#6x
>       2 %#4x
>       2 %#10
>       2 %#09x
>       2 %#05lx
>       1 %#8lx
>       1 %#5x
>       1 %#5lx
>       1 %#2Lx
>       1 %#2llx
>       1 %#16x
>       1 %#16lx
>       1 %#12x
>       1 %#0x10000
>       1 %#0lx
>       1 %#08
>       1 %#05llx
>       1 %#04o
>       1 %#04lx
>       1 %#03X
>       1 %#018lx
>       1 %#010zx
> 
> ---
>  scripts/checkpatch.pl | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d65334588eb4c..5840f3f2aee6f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6695,6 +6695,31 @@ sub process {
>  				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
>  				$fmt =~ s/%%//g;
>  
> +				while ($fmt =~ /\%#([\d]+)/g) {
> +					my $length = $1;
> +					my $pref_len;
> +					if ($length < 4) {
> +						$pref_len = '04';
> +					} elsif ($length == 5) {
> +						$pref_len = '06';
> +					} elsif ($length > 6 && $length < 10) {
> +						$pref_len = '010';
> +					} elsif ($length > 10 && $length < 18) {
> +						$pref_len = '018';
> +					} elsif ($length > 18) {
> +						$pref_len = '<something else>';
> +					}
> +					if (defined($pref_len)) {
> +						if (!defined($stat_real)) {
> +							$stat_real = get_stat_real($linenr, $lc);
> +						}
> +						WARN("VSPRINTF_SPECIAL_LENGTH",
> +						     "Unusual special length '%#$length' in 0x prefixed output, length is usually 2 more than the desired width - perhaps use '%#${pref_len}'\n" . "$here\n$stat_real");
> +					}
> +				}
> +
> +				pos($fmt) = 0;
> +
>  				while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) {
>  					$specifier = $1;
>  					$extension = $2;
>