Re: [PATCH] kref: Avoid null pointer dereference after WARN - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Kees Cook <keescook@chromium.org>
Cc: Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	"Jason A. Donenfeld" <Jason@zx2c4.com>,
	Thomas Hellstrom <thellstrom@vmware.com>,
	Andi Kleen <ak@linux.intel.com>,
	Daniel Micay <danielmicay@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kref: Avoid null pointer dereference after WARN
Date: Wed, 28 Jun 2017 13:57:33 +0200	[thread overview]
Message-ID: <20170628115733.GA9737@kroah.com> (raw)
In-Reply-To: <20170627035215.GA132342@beast>

On Mon, Jun 26, 2017 at 08:52:15PM -0700, Kees Cook wrote:
> From: Daniel Micay <danielmicay@gmail.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