[PATCH] kref: Avoid null pointer dereference after WARN

[PATCH] kref: Avoid null pointer dereference after WARN

[Kees Cook]

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 kref_put() case is extracted from PaX, and Kees Cook noted it should
be extended to the other two cases.

Signed-off-by: Daniel Micay <danielmicay@gmail.com>
[kees: clarify commit log]
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/kref.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index f4156f88f557..82a2c225eae3 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -66,7 +66,7 @@ static inline void kref_get(struct kref *kref)
  */
 static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref))
 {
-	WARN_ON(release == NULL);
+	BUG_ON(release == NULL);
 
 	if (refcount_dec_and_test(&kref->refcount)) {
 		release(kref);
@@ -79,7 +79,7 @@ static inline int kref_put_mutex(struct kref *kref,
 				 void (*release)(struct kref *kref),
 				 struct mutex *lock)
 {
-	WARN_ON(release == NULL);
+	BUG_ON(release == NULL);
 
 	if (refcount_dec_and_mutex_lock(&kref->refcount, lock)) {
 		release(kref);
@@ -92,7 +92,7 @@ static inline int kref_put_lock(struct kref *kref,
 				void (*release)(struct kref *kref),
 				spinlock_t *lock)
 {
-	WARN_ON(release == NULL);
+	BUG_ON(release == NULL);
 
 	if (refcount_dec_and_lock(&kref->refcount, lock)) {
 		release(kref);

-- 
Kees Cook
Pixel Security

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

[Greg Kroah-Hartman]

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 kref_put() case is extracted from PaX, and Kees Cook noted it should
> be extended to the other two cases.
> 
> Signed-off-by: Daniel Micay <danielmicay@gmail.com>
> [kees: clarify commit log]
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/kref.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/kref.h b/include/linux/kref.h
> index f4156f88f557..82a2c225eae3 100644
> --- a/include/linux/kref.h
> +++ b/include/linux/kref.h
> @@ -66,7 +66,7 @@ static inline void kref_get(struct kref *kref)
>   */
>  static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref))
>  {
> -	WARN_ON(release == NULL);
> +	BUG_ON(release == NULL);

I remember one complaint was that WARN_ON was "huge" and this bloated
the kernel code a lot.  But then that got fixed up.  Is BUG_ON going to
cause the same complaint again?

thanks,

greg k-h

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

[Jason A. Donenfeld]

On Tue, Jun 27, 2017 at 09:06:26AM +0200, Greg Kroah-Hartman wrote:
> I remember one complaint was that WARN_ON was "huge" and this bloated
> the kernel code a lot.  But then that got fixed up.  Is BUG_ON going to
> cause the same complaint again?

Complaint or not, I'm pretty sure using BUG_ON here is the right
behavior. If removing it will result in a null pointer dereference (and
subsequent function call), that's bad news bears, especially on systems
with a zero mmap_min_addr or combined with other bugs.

If somehow a driver manages to pass a NULL as the release function,
something is really messed up and the kernel should "safely" panic
instead.

> 
> thanks,
> 
> greg k-h

-- 
Jason A. Donenfeld
Deep Space Explorer
fr: +33 6 51 90 82 66
us: +1 513 476 1200
www.jasondonenfeld.com
www.zx2c4.com
zx2c4.com/keys/AB9942E6D4A4CFC3412620A749FC7012A5DE03AE.asc

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

[Andi Kleen]

On Tue, Jun 27, 2017 at 01:00:16PM +0200, Jason A. Donenfeld wrote:
> On Tue, Jun 27, 2017 at 09:06:26AM +0200, Greg Kroah-Hartman wrote:
> > I remember one complaint was that WARN_ON was "huge" and this bloated
> > the kernel code a lot.  But then that got fixed up.  Is BUG_ON going to
> > cause the same complaint again?
> 
> Complaint or not, I'm pretty sure using BUG_ON here is the right
> behavior. If removing it will result in a null pointer dereference (and
> subsequent function call), that's bad news bears, especially on systems
> with a zero mmap_min_addr or combined with other bugs.
> 
> If somehow a driver manages to pass a NULL as the release function,
> something is really messed up and the kernel should "safely" panic
> instead.

Please post data on the kernel text change from this patch.

In general I'm sceptical on the value of changes like this. Why
is release so important to check compared to other places?

Is there any data how many security holes this would have
caught? Please no hand waving. A lot of the recent
security patches seem to have gone in with just a lot of
hand waving and security theater 

("It's in gr-security, so it must be great")

In general accountability is important, especially in security

-Andi

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

[Jason A. Donenfeld]

On Tue, Jun 27, 2017 at 4:49 PM, Andi Kleen <ak@linux.intel.com> wrote:
> Is there any data how many security holes this would have
> caught? Please no hand waving. A lot of the recent
> security patches seem to have gone in with just a lot of
> hand waving and security theater

I don't practice security theater. What an offensive insinuation.
Maybe you just meant this about other patches, however.

The point was that if there prior was a WARN_ON, this needs to be a
BUG_ON, since if the WARN_ON was put there with any validity,
continuing after it will always be "fatal and potentially
exploitable". Thus, it'd be better to change that to simply "fatal but
not potentially exploitable". Not security theater. Logic fix.

The bigger question, though, is the value of these checks in the first
place. Has anybody written a coccinelle check to look into this
statically? Has it historically been a useful thing for driver
developers to have? Is it good defense in depth or is it overkill? At
the very least, the original authors of kref thought a WARN_ON was
warranted, which means probably a BUG_ON is a sensible fix, until
somebody does the work of investigating these more careful questions.

Jason

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

[Andi Kleen]

On Tue, Jun 27, 2017 at 09:11:28PM +0200, Jason A. Donenfeld wrote:
> On Tue, Jun 27, 2017 at 4:49 PM, Andi Kleen <ak@linux.intel.com> wrote:
> > Is there any data how many security holes this would have
> > caught? Please no hand waving. A lot of the recent
> > security patches seem to have gone in with just a lot of
> > hand waving and security theater
> 
> I don't practice security theater. What an offensive insinuation.
> Maybe you just meant this about other patches, however.

I'm not naming names, but there was a recent patch
that seemed to have fixed one very extremely specific bug,
but made every kernel exit forever slower.

That was a classic case IMHO -- the Linux equivalent of
shoes at airport checkpoints.

> The point was that if there prior was a WARN_ON, this needs to be a
> BUG_ON, since if the WARN_ON was put there with any validity,
> continuing after it will always be "fatal and potentially
> exploitable". Thus, it'd be better to change that to simply "fatal but

That's not necessarily true. Especially not for a release.
Typically you would hit if partial teardown on an initialization
failure is incorrect. But that's not exploitable at all.

> The bigger question, though, is the value of these checks in the first
> place. Has anybody written a coccinelle check to look into this
> statically? Has it historically been a useful thing for driver
> developers to have? Is it good defense in depth or is it overkill? At
> the very least, the original authors of kref thought a WARN_ON was
> warranted, which means probably a BUG_ON is a sensible fix, until
> somebody does the work of investigating these more careful questions.

Right that's the question that should have been answered before
this patch.

I don't think it was ever intended to be a defense, just as a hint
for driver developers.

My suspicion is that they're mostly useless.

-Andi

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

[Michal Hocko]

On Tue 27-06-17 12:29:39, Andi Kleen wrote:
> On Tue, Jun 27, 2017 at 09:11:28PM +0200, Jason A. Donenfeld wrote:
[...]
> > The bigger question, though, is the value of these checks in the first
> > place. Has anybody written a coccinelle check to look into this
> > statically? Has it historically been a useful thing for driver
> > developers to have? Is it good defense in depth or is it overkill? At
> > the very least, the original authors of kref thought a WARN_ON was
> > warranted, which means probably a BUG_ON is a sensible fix, until
> > somebody does the work of investigating these more careful questions.
> 
> Right that's the question that should have been answered before
> this patch.
> 
> I don't think it was ever intended to be a defense, just as a hint
> for driver developers.
> 
> My suspicion is that they're mostly useless.

Completely agreed here!
-- 
Michal Hocko
SUSE Labs

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

[Kees Cook]

On Tue, Jun 27, 2017 at 12:06 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 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 kref_put() case is extracted from PaX, and Kees Cook noted it should
>> be extended to the other two cases.
>>
>> Signed-off-by: Daniel Micay <danielmicay@gmail.com>
>> [kees: clarify commit log]
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  include/linux/kref.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/kref.h b/include/linux/kref.h
>> index f4156f88f557..82a2c225eae3 100644
>> --- a/include/linux/kref.h
>> +++ b/include/linux/kref.h
>> @@ -66,7 +66,7 @@ static inline void kref_get(struct kref *kref)
>>   */
>>  static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref))
>>  {
>> -     WARN_ON(release == NULL);
>> +     BUG_ON(release == NULL);
>
> I remember one complaint was that WARN_ON was "huge" and this bloated
> the kernel code a lot.  But then that got fixed up.  Is BUG_ON going to
> cause the same complaint again?

It used to be that WARN was much larger than BUG, but Peter Z fixed[1]
that recently. BUG is very small (on x86 a single instruction). I'll
send a v2 with the .text comparison.

-Kees

[1] 9a93848fe787 ("x86/debug: Implement __WARN() using UD0")

-- 
Kees Cook
Pixel Security

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

[Peter Zijlstra]

On Tue, Jun 27, 2017 at 09:06:26AM +0200, Greg Kroah-Hartman wrote:
> 
> I remember one complaint was that WARN_ON was "huge" and this bloated
> the kernel code a lot.  But then that got fixed up.  Is BUG_ON going to
> cause the same complaint again?

BUG_ON and WARN_ON should be of equal (small) size.

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

[Greg Kroah-Hartman]

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