linux-kernel - Re: [PATCH]: Cleanup: Remove gcc format string warnings when compiling with -Wformat-security

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: <56e1b5710902051441g69b2df61n6f6b77a3dc59ac42@mail.gmail.com>
Date:	Thu, 5 Feb 2009 23:41:34 +0100
From:	Floris Kraak <randakar@...il.com>
To:	Roland Dreier <rdreier@...co.com>
Cc:	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

On Thu, Feb 5, 2009 at 11:13 PM, Roland Dreier <rdreier@...co.com> wrote:
>
> I would strongly prefer to do this with a little more care.  For example
> the b43/main.c change:
>
>  > --- a/drivers/net/wireless/b43/main.c
>  > +++ b/drivers/net/wireless/b43/main.c
>  > @@ -2005,9 +2005,9 @@ static void b43_print_fw_helptext(struct b43_wl
>  > *wl, bool error)
>  >             "http://linuxwireless.org/en/users/Drivers/b43#devicefirmware "
>  >             "and download the latest firmware (version 4).\n";
>  >      if (error)
>  > -            b43err(wl, text);
>  > +            b43err(wl, "%s", text);
>  >      else
>  > -            b43warn(wl, text);
>  > +            b43warn(wl, "%s", text);
>  >  }
> would probably be better solved by doing
>
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -1999,11 +1999,11 @@ static void b43_release_firmware(struct b43_wldev *dev)
>
>  static void b43_print_fw_helptext(struct b43_wl *wl, bool error)
>  {
> -       const char *text;
> +       static const char text[] =
> +               "You must go to "
> +               "http://linuxwireless.org/en/users/Drivers/b43#devicefirmware "
> +               "and download the latest firmware (version 4).\n";
>
> -       text = "You must go to "
> -              "http://linuxwireless.org/en/users/Drivers/b43#devicefirmware "
> -              "and download the latest firmware (version 4).\n";
>        if (error)
>                b43err(wl, text);
>        else
>

Fair enough. I'm going to have to break this down into categories of
bugs probably anyway. - "trivial just makes the compiler stops
whining" vs "omg security hole" and the shades in between. From what
I've seen so far the vast majority of this falls into the first
category though ;-)

> and in any case I'm not totally convinced that we want to add the bloat
> for trivial cases like
>
>        char *safe = "foo";
>        printk(safe);
>
> Would be nice to think of a cleverer way to handle that...
>

It would help if GCC was a little smarter in the detection, right now
it gives a lot of false positives even on code like that.
When I was playing around with coccinelle tonight in a (probably
misguided) attempt to automate this I found a few new possible cases
where macro wrappers around printk() seem to make avoiding this check
possible. Haven't looked deeply yet, I'm doing this on my spare time
which I don't have tons of ;-)

Regards,
Floris
---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson
--
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/