Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753496AbdF0UQv (ORCPT ); Tue, 27 Jun 2017 16:16:51 -0400 Received: from mail-it0-f65.google.com ([209.85.214.65]:32892 "EHLO mail-it0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753087AbdF0UQn (ORCPT ); Tue, 27 Jun 2017 16:16:43 -0400 Message-ID: <1498594600.1180.2.camel@gmail.com> Subject: Re: [PATCH v2] kref: Avoid null pointer dereference after WARN From: Daniel Micay To: Kees Cook , "Jason A. Donenfeld" Cc: Andi Kleen , Greg Kroah-Hartman , Ingo Molnar , Peter Zijlstra , Thomas Hellstrom , LKML Date: Tue, 27 Jun 2017 16:16:40 -0400 In-Reply-To: References: <20170627190001.GA7811@beast> <20170627192242.GI23705@tassilo.jf.intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.24.3 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1966 Lines: 50 On Tue, 2017-06-27 at 12:34 -0700, Kees Cook wrote: > On Tue, Jun 27, 2017 at 12:26 PM, Jason A. Donenfeld > wrote: > > On Tue, Jun 27, 2017 at 9:22 PM, Andi Kleen > > wrote: > > > Who would actually set mman_min_addr incorrectly? > > > > Historically there have been quite a few bypasses of mmap_min_addr, > > actually. This is well-trodden ground. > > Targeting things in /proc/sys via confused privileged helpers is > extremely common. See Chrome OS pwn2own exploits (targetting modprobe > sysctl), and plenty of others. Modern attack methodology is rarely a > single-bug attack, but rather a chain of bugs, which may include > producing or exploiting weak userspace configurations to soften the > kernel. > > Regardless, it's a fair point that checking this unconditionally is > wasteful. Strangely this doesn't help: > > - BUG_ON(release == NULL); > + if (!__builtin_constant_p(release)) > + BUG_ON(release == NULL); > > When nearly all callers pass a function directly: > > ... > drivers/block/rbd.c: kref_put(&spec->kref, rbd_spec_free); > drivers/char/hw_random/core.c: kref_put(&rng->ref, > cleanup_rng); > drivers/char/ipmi/ipmi_msghandler.c: > kref_put(&e->intf->refcount, intf_free); > ... > > Hmmm > > -Kees It doesn't mean the address is constant if there's a fixed function being passed to it. It's not known at compile-time and if the code can be relocated it's not known at link-time. I don't personally care about checks like this but I split it out with some others just because it was there already. Clang has a nullability attribute which is similar to nonnull but it doesn't cause UB when violated, so if GCC picked that up it could be added all over the place as an annotation on parameters to trigger warnings. There's a sanitizer for it, so it can be made to trap with -fsanitize=nullability -fsanitize-trap=nullability.