linux-kernel - Re: [PATCH] kref: Avoid null pointer dereference after WARN

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, 28 Jun 2017 13:57:33 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Daniel Micay <danielmicay@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kref: Avoid null pointer dereference after WARN

On Mon, Jun 26, 2017 at 08:52:15PM -0700, Kees Cook wrote:
> From: Daniel Micay <danielmicay@...il.com>
> 
> The WARN_ON() checking for a NULL release pointer should be a BUG()
> since continuing with a NULL release pointer will lead to a NULL
> pointer dereference anyway.

The reason I enforced the release pointer not being NULL in the first
place, was because to not do so is a logic bug, not a run-time bug.

By putting the WARN_ON there, developers get to see the traceback and
know to fix their code.  The driver core is even harsher printing out a
message if the release function is not set, and doing a full WARN_ON().

When you just crash the machine, with BUG(), you are preventing the
developer to have a chance to know what to fix.  With WARN_ON, the
machine still runs, and they can fix up their code.  Odds are, when
someone doesn't provide a release function, their logic is such that it
will never be called anyway as their reference counting is totally
broken anyway :)

So this is a helper for the developer, let's not turn it into something
that is then a "crash the machine" issue.

Now if we could also come up with a way for the check to see if the
release function was something foolish like:
	void my_release(kref *kref) { }
that would be ideal, as the documentation explicitly says not to do
this, and I will be able to make fun of you in public if you do, yet I
constantly see _that_ happen all the time..

So let's leave this as WARN_ON(), it's not a security issue / protection
issue in any sense of the word.  It's a "let's help provide an API that
tells the developer when they use it incorrectly" type thing.

Also, you will note, all calls to kref_put() in the kernel tree do not
have this issue, so really, one could argue that this check is doing its
job :)

So I'll just drop this patch for now, thanks,

greg k-h