linux-kernel - Re: [PATCH] fortify: Use WARN instead of BUG for now

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]

Date:   Wed, 26 Jul 2017 10:21:39 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Daniel Micay <danielmicay@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        David Howells <dhowells@...hat.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fortify: Use WARN instead of BUG for now

On Wed, Jul 26, 2017 at 5:52 AM, Daniel Micay <danielmicay@...il.com> wrote:
> It should just be renamed from fortify_panic -> fortify_error, including
> in arch/x86/boot/compressed/misc.c and arch/x86/boot/compressed/misc.c.

Somehow I missed these. I'll send a v2. I wonder why those didn't trip
in my build...

> It can use WARN instead of BUG by with a 'default n', !COMPILE_TEST
> option to use BUG again. Otherwise it needs to be patched downstream
> when that's wanted.

I figure that'll be a separate conversation. For now, we'll do WARN.

> I don't think splitting it is the right approach to improving the
> runtime error handling. That only makes sense for the compile-time
> errors due to the limitations of __attribute__((error)). Can we think
> about that before changing it? Just make it use WARN for now.

Part of Linus's objection was the vague error report. Since the split
didn't bloat things very much, it seemed okay to me.

> The best debugging experience would be passing along the sizes and
> having the fortify_error function convert that into nice error messages.
> For memcpy(p, q, n), n can be larger than both the detected sizes of p
> and q, not just either one. The error should just be saying the function
> name and printing the copy size and maximum sizes of p and q. That's
> going to increase the code size too but I think splitting it will be
> worse and it goes in the wrong direction in terms of complexity. It's
> going to make future extensions / optimization harder if it's split.

Maybe we could do two phases? One to s/BUG/WARN/ and the second to
improve the message?

-Kees

-- 
Kees Cook
Pixel Security