[PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael J Dilmore]

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@gmail.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);
 
 		if (new_active == old_active) {
 			/* do nothing */
-- 
2.7.4

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Jay Vosburgh]

Michael J Dilmore <michael.j.dilmore@gmail.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@gmail.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@canonical.com

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael D]

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@canonical.com> wrote:
> Michael J Dilmore <michael.j.dilmore@gmail.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@gmail.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@canonical.com

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[David Miller]

From: Michael D <michael.j.dilmore@gmail.com>
Date: Wed, 21 Jun 2017 22:35:56 +0100

> How do we actually stop a null pointer being dereferenced here?

Maybe that's why it's a BUG_ON(), there is no reasonable way
to continue if the pointer is NULL.

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael D]

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?





On 21 June 2017 at 22:36, David Miller <davem@davemloft.net> wrote:
> From: Michael D <michael.j.dilmore@gmail.com>
> Date: Wed, 21 Jun 2017 22:35:56 +0100
>
>> How do we actually stop a null pointer being dereferenced here?
>
> Maybe that's why it's a BUG_ON(), there is no reasonable way
> to continue if the pointer is NULL.
>

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[David Miller]

From: Michael D <michael.j.dilmore@gmail.com>
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.

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[David Miller]

And please stop top posting, thank you.

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael J Dilmore]

On 21/06/17 22:56, David Miller wrote:

> From: Michael D <michael.j.dilmore@gmail.com>
> 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
}

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: Michael D <michael.j.dilmore@gmail.com>
>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.

	Looking at the code more carefully than I did earlier, the only
way the BUG_ON will hit is if the rx_handler_data is NULL for a bonding
slave when this code executes.

	This should be impossible, as there doesn't appear to be any way
to get into bond_option_active_slave_set for a slave prior to
bond_enslave registering the rx_handler for that slave, as these
operations are mutexed by RTNL.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Jay Vosburgh]

Michael J Dilmore <michael.j.dilmore@gmail.com> wrote:

>On 21/06/17 22:56, David Miller wrote:
>
>> From: Michael D <michael.j.dilmore@gmail.com>
>> 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

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael J Dilmore]

On 21/06/17 23:39, Jay Vosburgh wrote:
> Michael J Dilmore <michael.j.dilmore@gmail.com> wrote:
>
>> On 21/06/17 22:56, David Miller wrote:
>>
>>> From: Michael D <michael.j.dilmore@gmail.com>
>>> 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
That did cross my mind but I read that Linus that was quite averse to 
BUG_ON anywhere in the kernel so thought it might be have been worth doing.

Is it worth at least wrapping BUG_ON in an unlikely macro then?

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michal Kubecek]

On Thu, Jun 22, 2017 at 12:04:54AM +0100, Michael J Dilmore wrote:
> 
> Is it worth at least wrapping BUG_ON in an unlikely macro then?

See BUG_ON() definition:

#ifndef HAVE_ARCH_BUG_ON
#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
#endif

where HAVE_ARCH_BUG_ON is defined only on powerpc and mips. It makes
good sense, you don't want to BUG_ON() on a condition unless it's
extremely unlikely. (Except for debugging purpose but even then you
don't really care about fine optimization when you are going to oops.)

Michal Kubecek

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Bjørn Mork]

Jay Vosburgh <jay.vosburgh@canonical.com> writes:
> Michael J Dilmore <michael.j.dilmore@gmail.com> wrote:
>
>>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 it really is impossible then you should simply remove the test. No
need to test for the impossible, is there?


Bjørn