linux-kernel - Re: [PATCH v3] led/led-class: Handle LEDs with the same name

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: <CAGXu5jLPRS+0KOcb2g1y_1Yqg-NywshHiujxNghQJ5HdFVWo1Q@mail.gmail.com>
Date:	Mon, 8 Jun 2015 15:55:45 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Fengguang Wu <fengguang.wu@...el.com>
Cc:	Bryan Wu <cooloney@...il.com>,
	Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>,
	Sakari Ailus <sakari.ailus@....fi>,
	Richard Purdie <rpurdie@...ys.net>,
	Linux LED Subsystem <linux-leds@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Christopher Li <sparse@...isli.org>
Subject: Re: [PATCH v3] led/led-class: Handle LEDs with the same name

On Fri, Mar 27, 2015 at 6:07 PM, Fengguang Wu <fengguang.wu@...el.com> wrote:
> + Kees Cook
>
> On Fri, Mar 27, 2015 at 10:24:53AM -0700, Bryan Wu wrote:
>> On Fri, Mar 27, 2015 at 1:09 AM, Ricardo Ribalda Delgado
>> <ricardo.ribalda@...il.com> wrote:
>> > Hi Sakari
>> >
>> > cc: adding Greg (core and FormatGuard) and Chistopher (sparse)
>> >>
>> >> I just realised there was another issue --- the name is now interpreted as
>> >> format string. Bad things will happen if there's e.g. %s in the name itself
>> >> --- perhaps unlikely, but possible.
>> >
>> > Good catch!
>> >
>> > Would it be possible to add a sparse check to avoid this in all the kernel?
>> >
>> > And what about a macro protection like FormatGuard?
>> >
>> > https://www.usenix.org/legacy/events/sec01/full_papers/cowanbarringer/cowanbarringer.pdf
>> >
>> >
>>
>> I think Fengguang's 0-DAY kernel test infrastructure can help this.
>
> Kees' format-security branch has a check on dynamic printf format
> string, which has been effective in finding errors like:
>
>    drivers/tty/serial/sb1250-duart.c: In function 'sbd_map_port':
>>> drivers/tty/serial/sb1250-duart.c:680:3: error: format not a string literal and no format arguments [-Werror=format-security]
>       printk(err);
>       ^
>
> I wonder if Kees has the plan to include the patch into upstream and
> make it a kconfig option. For your convenience, the patch is pasted
> below.
>
> Thanks,
> Fengguang
> ---
>
> commit 95420c349194d1b570270ba1b1567d85461761c3
> Author:     Kees Cook <keescook@...omium.org>
> AuthorDate: Mon Sep 16 11:15:54 2013 -0700
> Commit:     Kees Cook <keescook@...omium.org>
> CommitDate: Wed Mar 4 14:07:18 2015 -0800
>
>     Make all format string problems fail the build
>
>     In an effort to stop format strings from leaking into various callers,
>     have gcc stop the build when this gets detected.
>
>     Signed-off-by: Kees Cook <keescook@...omium.org>
>
> diff --git a/Makefile b/Makefile
> index e6a9b1b..b7684d2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -402,7 +402,6 @@ KBUILD_CPPFLAGS := -D__KERNEL__
>  KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>                    -fno-strict-aliasing -fno-common \
>                    -Werror-implicit-function-declaration \
> -                  -Wno-format-security \
>                    -std=gnu89
>
>  KBUILD_AFLAGS_KERNEL :=
> @@ -752,6 +751,11 @@ endif
>  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>  CHECKFLAGS     += $(NOSTDINC_FLAGS)
>
> +# Enable format-security when it can stop the build, otherwise disable.
> +KBUILD_CFLAGS  += $(call cc-option,\
> +                       -Wformat -Wformat-security -Werror=format-security,\
> +                       -Wno-format-security)
> +
>  # warn about C99 declaration after statement
>  KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)
>

The trouble is that gcc still has a terrible time with false positives
on these warnings. It doesn't know to silence const char strings. For
example:

const char * version = "SomeThing driver v1.3\n";
...
printk(version);

We don't care about this, since the string is not dynamic, but gcc
still warns. My intention is that when gcc fixes this bug, then I'd
upstream this patch. Right now I have to carry a patch to silence
false positives. :(

You can see it here:
http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=format-security

-Kees

-- 
Kees Cook
Chrome OS Security
--
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/