netdev - Re: [PATCH] Convert BUG_ON to WARN_ON in bond

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALRmy5sYL_gJsiLzQQn-2FbWowi=Bj4DkYdcOdrOHeSs4jQ3Xg@mail.gmail.com>
Date:   Wed, 21 Jun 2017 22:35:56 +0100
From:   Michael D <michael.j.dilmore@...il.com>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Joe Perches <joe@...ches.com>
Subject: Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

How do we actually stop a null pointer being dereferenced here? Other
examples I've seen check for null and then immediately return the
function with an error code so that it cannot be referenced again.
Something like:

if (WARN_ON(!new_active)
      return 1

This behaviour should be OK for this function, as if active_slave is
null, a new active slave obviously cannot be set. Or is there

On 21 June 2017 at 19:33, Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
> Michael J Dilmore <michael.j.dilmore@...il.com> wrote:
>
>>The function below contains a BUG_ON where no active slave is detected. The patch
>>converts this to a WARN_ON to avoid crashing the kernel.
>>
>>Signed-off-by: Michael J Dilmore <michael.j.dilmore@...il.com>
>>---
>> drivers/net/bonding/bond_options.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>>index 1bcbb89..c4b4791 100644
>>--- a/drivers/net/bonding/bond_options.c
>>+++ b/drivers/net/bonding/bond_options.c
>>@@ -778,7 +778,7 @@ static int bond_option_active_slave_set(struct bonding *bond,
>>               struct slave *old_active = rtnl_dereference(bond->curr_active_slave);
>>               struct slave *new_active = bond_slave_get_rtnl(slave_dev);
>>
>>-              BUG_ON(!new_active);
>>+              WARN_ON(!new_active);
>
>         This is a reasonable idea in principle, but will require
> additional changes to prevent dereferencing new_active if it is NULL
> (which would happen just below this point in the code).
>
>         -J
>
>>               if (new_active == old_active) {
>>                       /* do nothing */
>>--
>>2.7.4
>>
>
> ---
>         -Jay Vosburgh, jay.vosburgh@...onical.com