Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752208AbdFUWjj (ORCPT ); Wed, 21 Jun 2017 18:39:39 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:55417 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751963AbdFUWji (ORCPT ); Wed, 21 Jun 2017 18:39:38 -0400 From: Jay Vosburgh To: Michael J Dilmore cc: David Miller , vfalico@gmail.com, andy@greyhouse.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, joe@perches.com Subject: Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c In-reply-to: <125b4ae9-2cb7-3532-5391-24404cf6eaec@gmail.com> References: <20170621.173655.1945994342723484710.davem@davemloft.net> <20170621.175651.854625612625047729.davem@davemloft.net> <125b4ae9-2cb7-3532-5391-24404cf6eaec@gmail.com> Comments: In-reply-to Michael J Dilmore message dated "Wed, 21 Jun 2017 23:27:41 +0100." X-Mailer: MH-E 8.6+git-jv; nmh 1.6; GNU Emacs 25.1.50 MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <24690.1498084768.1@famine> Date: Wed, 21 Jun 2017 15:39:28 -0700 Message-ID: <24691.1498084768@famine> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1417 Lines: 41 Michael J Dilmore wrote: >On 21/06/17 22:56, David Miller wrote: > >> From: Michael D >> Date: Wed, 21 Jun 2017 22:41:07 +0100 >> >>> I don't think you can stop it being dereferenced... you just need to >>> prevent an attacker from exploiting the null pointer dereference >>> vulnerability right? And this is done by returning the function right >>> away? >> What's all of this about an "attacker"? >> >> If there is a bug, we dererence a NULL pointer, and we should >> fix that bug. >> >> The BUG_ON() helps us see where the problem is while at the >> same time stopping the kernel before the NULL deref happens. >Ok this is starting to make sense now - went a bit off track but think my >general thinking is ok - i.e. if we return the function with an error code >before the dereference then this basically does the same thing as BUG_ON >but without crashing the kernel. > >Something like: > >if (WARN_ON(!new_active_slave) { > netdev_dbg("Can't add new active slave - pointer null"); > return ERROR_CODE >} In general, yes, but in this case, the condition should be impossible to hit, so BUG_ON seems appropriate. If bond_slave_get_rtnl/rcu() returns NULL for an actual bonding slave, other code paths (bond_fill_slave_info, bond_handle_frame) will likely crash before getting to this one. -J --- -Jay Vosburgh, jay.vosburgh@canonical.com