linux-kernel - Re: [PATCH] bug: Switch data corruption check to __must

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:   Tue, 7 Feb 2017 12:39:21 -0800
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
        mingo@...nel.org, jiangshanlai@...il.com, dipankar@...ibm.com,
        akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
        josh@...htriplett.org, tglx@...utronix.de, peterz@...radead.org,
        rostedt@...dmis.org, dhowells@...hat.com, edumazet@...gle.com,
        dvhart@...ux.intel.com, fweisbec@...il.com, oleg@...hat.com,
        bobby.prani@...il.com
Subject: Re: [PATCH] bug: Switch data corruption check to __must_check

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.

> +		bool corruption = unlikely(condition);			 \

So corruption = unlikely(condition)?  Sounds a bit optimistic to me!  ;-)

> +		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?

Still it is nice to avoid the magic return from out of the middle of the
C-preprocessor macro.

							Thanx, Paul

>  	return true;
>  }
> @@ -41,18 +42,20 @@ bool __list_del_entry_valid(struct list_head *entry)
>  	prev = entry->prev;
>  	next = entry->next;
> 
> -	CHECK_DATA_CORRUPTION(next == LIST_POISON1,
> -		"list_del corruption, %p->next is LIST_POISON1 (%p)\n",
> -		entry, LIST_POISON1);
> -	CHECK_DATA_CORRUPTION(prev == LIST_POISON2,
> -		"list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
> -		entry, LIST_POISON2);
> -	CHECK_DATA_CORRUPTION(prev->next != entry,
> -		"list_del corruption. prev->next should be %p, but was %p\n",
> -		entry, prev->next);
> -	CHECK_DATA_CORRUPTION(next->prev != entry,
> -		"list_del corruption. next->prev should be %p, but was %p\n",
> -		entry, next->prev);
> +	if (CHECK_DATA_CORRUPTION(next == LIST_POISON1,
> +			"list_del corruption, %p->next is LIST_POISON1 (%p)\n",
> +			entry, LIST_POISON1) ||
> +	    CHECK_DATA_CORRUPTION(prev == LIST_POISON2,
> +			"list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
> +			entry, LIST_POISON2) ||
> +	    CHECK_DATA_CORRUPTION(prev->next != entry,
> +			"list_del corruption. prev->next should be %p, but was %p\n",
> +			entry, prev->next) ||
> +	    CHECK_DATA_CORRUPTION(next->prev != entry,
> +			"list_del corruption. next->prev should be %p, but was %p\n",
> +			entry, next->prev))
> +		return false;
> +
>  	return true;
> 
>  }
> -- 
> 2.7.4
> 
> 
> -- 
> Kees Cook
> Pixel Security
>