linux-kernel - Re: [PATCH v3 3/3] list_debug: Introduce CONFIG_DEBUG_LIST

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: <20230809113021.63e5ef66@gandalf.local.home>
Date:   Wed, 9 Aug 2023 11:30:21 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Marco Elver <elver@...gle.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Peter Zijlstra <peterz@...radead.org>,
        Mark Rutland <mark.rutland@....com>,
        Marc Zyngier <maz@...nel.org>,
        Oliver Upton <oliver.upton@...ux.dev>,
        James Morse <james.morse@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Zenghui Yu <yuzenghui@...wei.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>, Miguel Ojeda <ojeda@...nel.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        kasan-dev@...glegroups.com, linux-toolchains@...r.kernel.org
Subject: Re: [PATCH v3 3/3] list_debug: Introduce CONFIG_DEBUG_LIST_MINIMAL

On Wed, 9 Aug 2023 11:57:19 +0200
Marco Elver <elver@...gle.com> wrote:

>  static __always_inline bool __list_add_valid(struct list_head *new,
>  					     struct list_head *prev,
>  					     struct list_head *next)
>  {
> -	return __list_add_valid_or_report(new, prev, next);
> +	bool ret = true;
> +
> +	if (IS_ENABLED(CONFIG_HARDEN_LIST)) {
> +		/*
> +		 * With the hardening version, elide checking if next and prev
> +		 * are NULL, since the immediate dereference of them below would
> +		 * result in a fault if NULL.
> +		 *
> +		 * With the reduced set of checks, we can afford to inline the
> +		 * checks, which also gives the compiler a chance to elide some
> +		 * of them completely if they can be proven at compile-time. If
> +		 * one of the pre-conditions does not hold, the slow-path will
> +		 * show a report which pre-condition failed.
> +		 */
> +		if (likely(next->prev == prev && prev->next == next && new != prev && new != next))
> +			return true;
> +		ret = false;
> +	}
> +
> +	ret &= __list_add_valid_or_report(new, prev, next);
> +	return ret;
>  }

I would actually prefer DEBUG_LIST to select HARDEN_LIST and not the other
way around. It logically doesn't make sense that HARDEN_LIST would select
DEBUG_LIST. That is, I could by default want HARDEN_LIST always on, but not
DEBUG_LIST (because who knows, it may add other features I don't want). But
then, I may have stumbled over something and want more info, and enable
DEBUG_LIST (while still having HARDEN_LIST) enabled.

I think you are looking at this from an implementation perspective and not
the normal developer one.

This would mean the above function should get enabled by CONFIG_HARDEN_LIST
(and CONFIG_DEBUG would select CONFIG_HARDEN) and would look more like:

static __always_inline bool __list_add_valid(struct list_head *new,
					     struct list_head *prev,
					     struct list_head *next)
{
	bool ret = true;

	if (!IS_ENABLED(CONFIG_DEBUG_LIST)) {
		/*
		 * With the hardening version, elide checking if next and prev
		 * are NULL, since the immediate dereference of them below would
		 * result in a fault if NULL.
		 *
		 * With the reduced set of checks, we can afford to inline the
		 * checks, which also gives the compiler a chance to elide some
		 * of them completely if they can be proven at compile-time. If
		 * one of the pre-conditions does not hold, the slow-path will
		 * show a report which pre-condition failed.
		 */
		if (likely(next->prev == prev && prev->next == next && new != prev && new != next))
			return true;
		ret = false;
	}

	ret &= __list_add_valid_or_report(new, prev, next);
	return ret;
}

That is, if DEBUG_LIST is enabled, we always call the
__list_add_valid_or_report(), but if only HARDEN_LIST is enabled, then we
do the shortcut.

-- Steve