From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752252AbdF1L7f (ORCPT ); Wed, 28 Jun 2017 07:59:35 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:54374 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751522AbdF1L7Z (ORCPT ); Wed, 28 Jun 2017 07:59:25 -0400 Date: Wed, 28 Jun 2017 13:57:33 +0200 From: Greg Kroah-Hartman To: Kees Cook Cc: Ingo Molnar , Peter Zijlstra , "Jason A. Donenfeld" , Thomas Hellstrom , Andi Kleen , Daniel Micay , linux-kernel@vger.kernel.org Subject: Re: [PATCH] kref: Avoid null pointer dereference after WARN Message-ID: <20170628115733.GA9737@kroah.com> References: <20170627035215.GA132342@beast> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170627035215.GA132342@beast> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 26, 2017 at 08:52:15PM -0700, Kees Cook wrote: > From: Daniel Micay > > 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