[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170628115733.GA9737@kroah.com>
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
Powered by blists - more mailing lists