| 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
| ||
|
Date: Tue, 7 Feb 2017 13:08:18 -0800
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Kees Cook <keescook@...omium.org>
Cc: Arnd Bergmann <arnd@...db.de>, LKML <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...nel.org>,
Lai Jiangshan <jiangshanlai@...il.com>, dipankar@...ibm.com,
Andrew Morton <akpm@...ux-foundation.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Josh Triplett <josh@...htriplett.org>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Steven Rostedt <rostedt@...dmis.org>,
David Howells <dhowells@...hat.com>,
Eric Dumazet <edumazet@...gle.com>,
Darren Hart <dvhart@...ux.intel.com>,
Frederic Weisbecker <fweisbec@...il.com>,
Oleg Nesterov <oleg@...hat.com>,
pranith kumar <bobby.prani@...il.com>
Subject: Re: [PATCH] bug: Switch data corruption check to __must_check
On Tue, Feb 07, 2017 at 12:57:33PM -0800, Kees Cook wrote:
> On Tue, Feb 7, 2017 at 12:39 PM, Paul E. McKenney
> <paulmck@...ux.vnet.ibm.com> wrote:
> > On Mon, Feb 06, 2017 at 12:45:47PM -0800, Kees Cook wrote:
> >> The CHECK_DATA_CORRUPTION() macro was designed to have callers do
> >> something meaningful/protective on failure. However, using "return false"
> >> in the macro too strictly limits the design patterns of callers. Instead,
> >> let callers handle the logic test directly, but make sure that the result
> >> IS checked by forcing __must_check (which appears to not be able to be
> >> used directly on macro expressions).
> >>
> >> Suggested-by: Arnd Bergmann <arnd@...db.de>
> >> Signed-off-by: Kees Cook <keescook@...omium.org>
> >> ---
> >> include/linux/bug.h | 12 +++++++-----
> >> lib/list_debug.c | 45 ++++++++++++++++++++++++---------------------
> >> 2 files changed, 31 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/include/linux/bug.h b/include/linux/bug.h
> >> index baff2e8fc8a8..5828489309bb 100644
> >> --- a/include/linux/bug.h
> >> +++ b/include/linux/bug.h
> >> @@ -124,18 +124,20 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
> >>
> >> /*
> >> * Since detected data corruption should stop operation on the affected
> >> - * structures, this returns false if the corruption condition is found.
> >> + * structures. Return value must be checked and sanely acted on by caller.
> >> */
> >> +static inline __must_check bool check_data_corruption(bool v) { return v; }
> >> #define CHECK_DATA_CORRUPTION(condition, fmt, ...) \
> >> - do { \
> >> - if (unlikely(condition)) { \
> >> + check_data_corruption(({ \
> >
> > The definition of check_data_corruption() is in some other patch? I don't
> > see it in current mainline. I am not seeing what it might be doing.
>
> It's immediately before the #define line above. It's nothing more than
> an inline argument pass-through, but since it's a _function_ I can
> attach __must_check to it, which I can't do for a conditional
> expression macro. And I gave it the meaningful name so when someone
> fails to check CHECK_DATA_CORRUPTION, they'll get a gcc warning about
> "check_data_corruption" which will lead them here.
Ah, I see it now. Color me blind!
Thanx, Paul
> >> + bool corruption = unlikely(condition); \
> >
> > So corruption = unlikely(condition)? Sounds a bit optimistic to me! ;-)
>
> It's true though! :) Nearly all calls to CHECK_DATA_CORRUPTION()
> should end up with a false condition.
>
> >
> >> + if (corruption) { \
> >> if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> >> pr_err(fmt, ##__VA_ARGS__); \
> >> BUG(); \
> >> } else \
> >> WARN(1, fmt, ##__VA_ARGS__); \
> >> - return false; \
> >> } \
> >> - } while (0)
> >> + corruption; \
> >> + }))
> >>
> >> #endif /* _LINUX_BUG_H */
> >> diff --git a/lib/list_debug.c b/lib/list_debug.c
> >> index 7f7bfa55eb6d..a34db8d27667 100644
> >> --- a/lib/list_debug.c
> >> +++ b/lib/list_debug.c
> >> @@ -20,15 +20,16 @@
> >> bool __list_add_valid(struct list_head *new, struct list_head *prev,
> >> struct list_head *next)
> >> {
> >> - CHECK_DATA_CORRUPTION(next->prev != prev,
> >> - "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n",
> >> - prev, next->prev, next);
> >> - CHECK_DATA_CORRUPTION(prev->next != next,
> >> - "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n",
> >> - next, prev->next, prev);
> >> - CHECK_DATA_CORRUPTION(new == prev || new == next,
> >> - "list_add double add: new=%p, prev=%p, next=%p.\n",
> >> - new, prev, next);
> >> + if (CHECK_DATA_CORRUPTION(next->prev != prev,
> >> + "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n",
> >> + prev, next->prev, next) ||
> >> + CHECK_DATA_CORRUPTION(prev->next != next,
> >> + "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n",
> >> + next, prev->next, prev) ||
> >> + CHECK_DATA_CORRUPTION(new == prev || new == next,
> >> + "list_add double add: new=%p, prev=%p, next=%p.\n",
> >> + new, prev, next))
> >> + return false;
> >
> > That -is- one ornate "if" condition, isn't it?
>
> It is, yes. :)
>
> > Still it is nice to avoid the magic return from out of the middle of the
> > C-preprocessor macro.
>
> Agreed. I had fun with indenting to make it passably readable. :P
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
>
Powered by blists - more mailing lists