| Message ID | 1498068137-3550-1-git-send-email-michael.j.dilmore@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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.
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. >
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.
And please stop top posting, thank you.
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 }
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
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
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?
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 <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
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 */
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(-)