kref: Avoid null pointer dereference after WARN

From: Daniel Micay <[email protected]>

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.

---
 include/linux/kref.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index f4156f88f557..82a2c225eae3 100644
@@ -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

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.

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

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

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

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.

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.

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

[...]

Completely agreed here!

Michal Hocko
SUSE Labs

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

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

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