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 <[email protected]>
---
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
Michael J Dilmore <[email protected]> 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 <[email protected]>
>---
> 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, [email protected]
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 <[email protected]> wrote:
> Michael J Dilmore <[email protected]> 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 <[email protected]>
>>---
>> 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, [email protected]
From: Michael D <[email protected]>
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.
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 <[email protected]> wrote:
> From: Michael D <[email protected]>
> 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.
>
From: Michael D <[email protected]>
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.
And please stop top posting, thank you.
On 21/06/17 22:56, David Miller wrote:
> From: Michael D <[email protected]>
> 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
}
David Miller <[email protected]> wrote:
>From: Michael D <[email protected]>
>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, [email protected]
Michael J Dilmore <[email protected]> wrote:
>On 21/06/17 22:56, David Miller wrote:
>
>> From: Michael D <[email protected]>
>> 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, [email protected]
On 21/06/17 23:39, Jay Vosburgh wrote:
> Michael J Dilmore <[email protected]> wrote:
>
>> On 21/06/17 22:56, David Miller wrote:
>>
>>> From: Michael D <[email protected]>
>>> 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, [email protected]
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?
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
Jay Vosburgh <[email protected]> writes:
> Michael J Dilmore <[email protected]> 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