linux-kernel - Re: [PATCH]: Cleanup: Remove gcc format string warnings when compiling with -Wformat-security (was: Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag)

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: <Pine.LNX.4.64.0902050908400.7489@cnc.isely.net>
Date:	Thu, 5 Feb 2009 09:22:25 -0600 (CST)
From:	Mike Isely <isely@...ly.net>
To:	Floris Kraak <randakar@...il.com>
cc:	Roland Dreier <rdreier@...co.com>,
	Robert Hancock <hancockrwd@...il.com>,
	Sam Ravnborg <sam@...nborg.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Trivial Patch Monkey <trivial@...nel.org>,
	Andreas Schwab <schwab@...e.de>
Subject: Re: [PATCH]: Cleanup: Remove gcc format string warnings when compiling
  with -Wformat-security (was: Re: [PATCH] Kbuild: Disable the  -Wformat-security
 gcc flag)

On Thu, 5 Feb 2009, Floris Kraak wrote:

   [...]

> 
> Here's the patch that I get when I blindly patch every single location
> that emits this warning.
> As noted before in some cases I'm not 100% sure this is the right way
> to go about it but it certainly is the KISS solution.
> 
> If needed I can attempt to split this monster into 135 patches but
> given my limited experience with the tools involved a little help on
> how to go about creating such a series would be appreciated ;-)
> 
> ---
> Cleanup: Remove gcc format string warnings when compiling with -Wformat-security
> 
> When compiling the kernel with an allyesconfig and the gcc flags
> -Wformat and -Wformat-security the build process emits 135 warnings
> along these lines:
> 
> init/main.c:557: warning: format not a string literal and no format arguments
> init/initramfs.c:582: warning: format not a string literal and no
> format arguments
> arch/x86/kernel/dumpstack.c:115: warning: format not a string literal
> and no format arguments
> ...
> 
> While many of these warnings are harmless - the format string is
> statically set within the kernel itself and is known to not contain
> any format qualifiers - a number of them are potentially less so.
> This patch fixes all known call sites emitting this warning.
> 
> Signed-off-by: Floris Kraak <randakar@...il.com>
> ---

   [...]

> diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> index fa304e5..42ccab0 100644
> --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> +++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> @@ -1967,7 +1967,7 @@ static void pvr2_hdw_setup_low(struct pvr2_hdw *hdw)
>  	if (!pvr2_hdw_dev_ok(hdw)) return;
> 
>  	for (idx = 0; idx < hdw->hdw_desc->client_modules.cnt; idx++) {
> -		request_module(hdw->hdw_desc->client_modules.lst[idx]);
> +		request_module("%s", hdw->hdw_desc->client_modules.lst[idx]);
>  	}

The incoming string in this case comes from a static initialization in 
the same source file (it's a compiled-in list of support module names to 
select based on the specific hardware type that the driver has 
detected).


> 
>  	if (!hdw->hdw_desc->flag_no_powerup) {
> diff --git a/drivers/media/video/pvrusb2/pvrusb2-std.c
> b/drivers/media/video/pvrusb2/pvrusb2-std.c
> index ca9f83a..c18091e 100644
> --- a/drivers/media/video/pvrusb2/pvrusb2-std.c
> +++ b/drivers/media/video/pvrusb2/pvrusb2-std.c
> @@ -216,7 +216,7 @@ unsigned int pvr2_std_id_to_str(char *bufPtr,
> unsigned int bufSize,
>  			bufSize -= c2;
>  			bufPtr += c2;
>  			c2 = scnprintf(bufPtr,bufSize,
> -				       ip->name);
> +				       "%s", ip->name);
>  			c1 += c2;
>  			bufSize -= c2;
>  			bufPtr += c2;

The pointer "ip" points into another array of structs, a member of which 
is a const string which is where the 'name' field comes from.  Thus once 
again there's no way an outside influence can inject a '%' here.

In both of these cases, the proposed change doesn't do any harm, but it 
does introduce some otherwise pointless inefficiency.

But hey, if this is all with the goal of silencing another script 
casting a big net, I won't stand in the way.

Acked-By: Mike Isely <isely@...ox.com>

  -Mike


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/