Re: [PATCH] net: Adding parameter detection in __ethtool_get_link_ksettings.
On Tue, Aug 27, 2019 at 07:01:41PM +, Saeed Mahameed wrote:
> On Mon, 2019-08-26 at 17:47 +0800, Dongxu Liu wrote:
>
> > Maybe "if (!dev->ethtool_ops)" is more accurate for this bug.
> >
>
> Also i am not sure about this, could be a bug in the device driver your
> enslaving.
>
> alloc_netdev_mqs will assign _ethtool_ops to dev->ethtool_ops ,
> if user provided setup callback didn't assign the driver specific
> ethtool_ops.
Dongxu said he encountered the null pointer dereference in a 3.10
kernel, not current mainline. But commit 2c60db037034 ("net: provide a
default dev->ethtool_ops") which introduced default_ethtool_ops came in
3.7-rc1 so 3.10 should have it already. There is indeed something wrong.
I don't think we should add either check unless we positively know that
dev->ethtool_ops can be null with current mainline kernel. And even
then, it would probably be more appropriate to fix the code which caused
it.
Michal
Re: [PATCH] net: Adding parameter detection in __ethtool_get_link_ksettings.
On Mon, 2019-08-26 at 17:47 +0800, Dongxu Liu wrote:
> > On 8/26/19 9:23 AM, Dongxu Liu wrote:
> > The __ethtool_get_link_ksettings symbol will be exported,
> > and external users may use an illegal address.
> > We should check the parameters before using them,
> > otherwise the system will crash.
> >
> > [ 8980.991134] BUG: unable to handle kernel NULL pointer
> > dereference at (null)
> > [ 8980.993049] IP: []
> > __ethtool_get_link_ksettings+0x27/0x140
> > [ 8980.994285] PGD 0
> > [ 8980.995013] Oops: [#1] SMP
> > [ 8980.995896] Modules linked in: sch_ingress ...
> > [ 8981.013220] CPU: 3 PID: 25174 Comm: kworker/3:3 Tainted:
> > G O V--- 3.10.0-327.36.58.4.x86_64 #1
> > [ 8981.017667] Workqueue: events linkwatch_event
> > [ 8981.018652] task: 8800a8348000 ti: 8800b045c000 task.ti:
> > 8800b045c000
> > [ 8981.020418] RIP: 0010:[] []
> > __ethtool_get_link_ksettings+0x27/0x140
> > [ 8981.022383] RSP: 0018:8800b045fc88 EFLAGS: 00010202
> > [ 8981.023453] RAX: RBX: 8800b045fcac RCX:
> >
> > [ 8981.024726] RDX: 8800b658f600 RSI: 8800b045fcac RDI:
> > 8802296e
> > [ 8981.026000] RBP: 8800b045fc98 R08: R09:
> > 0001
> > [ 8981.027273] R10: 73e0 R11: 082b0cc8adea R12:
> > 8802296e
> > [ 8981.028561] R13: 8800b566e8c0 R14: 8800b658f600 R15:
> > 8800b566e000
> > [ 8981.029841] FS: ()
> > GS:88023ed8() knlGS:
> > [ 8981.031715] CS: 0010 DS: ES: CR0: 80050033
> > [ 8981.032845] CR2: CR3: b39a9000 CR4:
> > 003407e0
> > [ 8981.034137] DR0: DR1: DR2:
> >
> > [ 8981.035427] DR3: DR6: fffe0ff0 DR7:
> > 0400
> > [ 8981.036702] Stack:
> > [ 8981.037406] 8800b658f600 9c40 8800b045fce8
> > a047a71d
> > [ 8981.039238] 004d 8800b045fcc8 8800b045fd28
> > 815cb198
> > [ 8981.041070] 8800b045fcd8 810807e6 e8212951
> > 0001
> > [ 8981.042910] Call Trace:
> > [ 8981.043660] []
> > bond_update_speed_duplex+0x3d/0x90 [bonding]
> > [ 8981.045424] [] ? inetdev_event+0x38/0x530
> > [ 8981.046554] [] ? put_online_cpus+0x56/0x80
> > [ 8981.047688] [] bond_netdev_event+0x137/0x360
> > [bonding]
> > ...
> >
> > Signed-off-by: Dongxu Liu
> > ---
> > net/core/ethtool.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> > 6288e69..9a50b64 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -545,6 +545,8 @@ int __ethtool_get_link_ksettings(struct
> > net_device
> > *dev, {
> > ASSERT_RTNL();
> >
> > + if (!dev || !dev->ethtool_ops)
> > + return -EOPNOTSUPP;
> > I do not believe dev can possibly be NULL at this point.
> > if (!dev->ethtool_ops->get_link_ksettings)
> > return -EOPNOTSUPP;
> >
> >
> > I tried to find an appropriate Fixes: tag.
> > It seems this particular bug was added either by
> > Fixes: 9856909c2abb ("net: bonding: use __ethtool_get_ksettings")
> > or generically in :
> > Fixes: 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS
> > API")
>
> In fact, "dev->ethtool_ops" is a null pointer in my environment.
> I didn't get the case where "dev" is a null pointer.
dev can't be a null pointer since bond driver guarantees that
and there is a check for the case where it could be null in
bond_slave_netdev_event.
You can drop the "!dev" check, since also it should be the caller
responsibility and we should avoid cluttering the net core code with
such redundant checks.
> Maybe "if (!dev->ethtool_ops)" is more accurate for this bug.
>
Also i am not sure about this, could be a bug in the device driver your
enslaving.
alloc_netdev_mqs will assign _ethtool_ops to dev->ethtool_ops ,
if user provided setup callback didn't assign the driver specific
ethtool_ops.
so the device driver must be doing something wrong, overwriting defult
ethtool_ops with a NULL pointer maybe ? and why ?
> I found this bug in version 3.10, the function name was
> __ethtool_get_settings.
> After 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS
> API"),
> This function evolved into __ethtool_get_link_ksettings.
>
Re: [PATCH] net: Adding parameter detection in __ethtool_get_link_ksettings.
> On 8/26/19 9:23 AM, Dongxu Liu wrote:
> The __ethtool_get_link_ksettings symbol will be exported,
> and external users may use an illegal address.
> We should check the parameters before using them,
> otherwise the system will crash.
>
> [ 8980.991134] BUG: unable to handle kernel NULL pointer dereference at
> (null)
> [ 8980.993049] IP: []
> __ethtool_get_link_ksettings+0x27/0x140
> [ 8980.994285] PGD 0
> [ 8980.995013] Oops: [#1] SMP
> [ 8980.995896] Modules linked in: sch_ingress ...
> [ 8981.013220] CPU: 3 PID: 25174 Comm: kworker/3:3 Tainted: G O
> V--- 3.10.0-327.36.58.4.x86_64 #1
> [ 8981.017667] Workqueue: events linkwatch_event
> [ 8981.018652] task: 8800a8348000 ti: 8800b045c000 task.ti:
> 8800b045c000
> [ 8981.020418] RIP: 0010:[] []
> __ethtool_get_link_ksettings+0x27/0x140
> [ 8981.022383] RSP: 0018:8800b045fc88 EFLAGS: 00010202
> [ 8981.023453] RAX: RBX: 8800b045fcac RCX:
>
> [ 8981.024726] RDX: 8800b658f600 RSI: 8800b045fcac RDI:
> 8802296e
> [ 8981.026000] RBP: 8800b045fc98 R08: R09:
> 0001
> [ 8981.027273] R10: 73e0 R11: 082b0cc8adea R12:
> 8802296e
> [ 8981.028561] R13: 8800b566e8c0 R14: 8800b658f600 R15:
> 8800b566e000
> [ 8981.029841] FS: () GS:88023ed8()
> knlGS:
> [ 8981.031715] CS: 0010 DS: ES: CR0: 80050033
> [ 8981.032845] CR2: CR3: b39a9000 CR4:
> 003407e0
> [ 8981.034137] DR0: DR1: DR2:
>
> [ 8981.035427] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 8981.036702] Stack:
> [ 8981.037406] 8800b658f600 9c40 8800b045fce8
> a047a71d
> [ 8981.039238] 004d 8800b045fcc8 8800b045fd28
> 815cb198
> [ 8981.041070] 8800b045fcd8 810807e6 e8212951
> 0001
> [ 8981.042910] Call Trace:
> [ 8981.043660] [] bond_update_speed_duplex+0x3d/0x90
> [bonding]
> [ 8981.045424] [] ? inetdev_event+0x38/0x530
> [ 8981.046554] [] ? put_online_cpus+0x56/0x80
> [ 8981.047688] [] bond_netdev_event+0x137/0x360 [bonding]
> ...
>
> Signed-off-by: Dongxu Liu
> ---
> net/core/ethtool.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> 6288e69..9a50b64 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -545,6 +545,8 @@ int __ethtool_get_link_ksettings(struct net_device
> *dev, {
> ASSERT_RTNL();
>
> + if (!dev || !dev->ethtool_ops)
> + return -EOPNOTSUPP;
> I do not believe dev can possibly be NULL at this point.
> if (!dev->ethtool_ops->get_link_ksettings)
> return -EOPNOTSUPP;
>
>
> I tried to find an appropriate Fixes: tag.
> It seems this particular bug was added either by
> Fixes: 9856909c2abb ("net: bonding: use __ethtool_get_ksettings")
> or generically in :
> Fixes: 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS API")
In fact, "dev->ethtool_ops" is a null pointer in my environment.
I didn't get the case where "dev" is a null pointer.
Maybe "if (!dev->ethtool_ops)" is more accurate for this bug.
I found this bug in version 3.10, the function name was __ethtool_get_settings.
After 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS API"),
This function evolved into __ethtool_get_link_ksettings.
Re: [PATCH] net: Adding parameter detection in __ethtool_get_link_ksettings.
On 8/26/19 9:23 AM, Dongxu Liu wrote:
> The __ethtool_get_link_ksettings symbol will be exported,
> and external users may use an illegal address.
> We should check the parameters before using them,
> otherwise the system will crash.
>
> [ 8980.991134] BUG: unable to handle kernel NULL pointer dereference at
> (null)
> [ 8980.993049] IP: []
> __ethtool_get_link_ksettings+0x27/0x140
> [ 8980.994285] PGD 0
> [ 8980.995013] Oops: [#1] SMP
> [ 8980.995896] Modules linked in: sch_ingress ...
> [ 8981.013220] CPU: 3 PID: 25174 Comm: kworker/3:3 Tainted: G O
> V--- 3.10.0-327.36.58.4.x86_64 #1
> [ 8981.017667] Workqueue: events linkwatch_event
> [ 8981.018652] task: 8800a8348000 ti: 8800b045c000 task.ti:
> 8800b045c000
> [ 8981.020418] RIP: 0010:[] []
> __ethtool_get_link_ksettings+0x27/0x140
> [ 8981.022383] RSP: 0018:8800b045fc88 EFLAGS: 00010202
> [ 8981.023453] RAX: RBX: 8800b045fcac RCX:
>
> [ 8981.024726] RDX: 8800b658f600 RSI: 8800b045fcac RDI:
> 8802296e
> [ 8981.026000] RBP: 8800b045fc98 R08: R09:
> 0001
> [ 8981.027273] R10: 73e0 R11: 082b0cc8adea R12:
> 8802296e
> [ 8981.028561] R13: 8800b566e8c0 R14: 8800b658f600 R15:
> 8800b566e000
> [ 8981.029841] FS: () GS:88023ed8()
> knlGS:
> [ 8981.031715] CS: 0010 DS: ES: CR0: 80050033
> [ 8981.032845] CR2: CR3: b39a9000 CR4:
> 003407e0
> [ 8981.034137] DR0: DR1: DR2:
>
> [ 8981.035427] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 8981.036702] Stack:
> [ 8981.037406] 8800b658f600 9c40 8800b045fce8
> a047a71d
> [ 8981.039238] 004d 8800b045fcc8 8800b045fd28
> 815cb198
> [ 8981.041070] 8800b045fcd8 810807e6 e8212951
> 0001
> [ 8981.042910] Call Trace:
> [ 8981.043660] [] bond_update_speed_duplex+0x3d/0x90
> [bonding]
> [ 8981.045424] [] ? inetdev_event+0x38/0x530
> [ 8981.046554] [] ? put_online_cpus+0x56/0x80
> [ 8981.047688] [] bond_netdev_event+0x137/0x360 [bonding]
> ...
>
> Signed-off-by: Dongxu Liu
> ---
> net/core/ethtool.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 6288e69..9a50b64 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -545,6 +545,8 @@ int __ethtool_get_link_ksettings(struct net_device *dev,
> {
> ASSERT_RTNL();
>
> + if (!dev || !dev->ethtool_ops)
> + return -EOPNOTSUPP;
I do not believe dev can possibly be NULL at this point.
> if (!dev->ethtool_ops->get_link_ksettings)
> return -EOPNOTSUPP;
>
>
I tried to find an appropriate Fixes: tag.
It seems this particular bug was added either by
Fixes: 9856909c2abb ("net: bonding: use __ethtool_get_ksettings")
or generically in :
Fixes: 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS API")
[PATCH] net: Adding parameter detection in __ethtool_get_link_ksettings.
The __ethtool_get_link_ksettings symbol will be exported,
and external users may use an illegal address.
We should check the parameters before using them,
otherwise the system will crash.
[ 8980.991134] BUG: unable to handle kernel NULL pointer dereference at
(null)
[ 8980.993049] IP: [] __ethtool_get_link_ksettings+0x27/0x140
[ 8980.994285] PGD 0
[ 8980.995013] Oops: [#1] SMP
[ 8980.995896] Modules linked in: sch_ingress ...
[ 8981.013220] CPU: 3 PID: 25174 Comm: kworker/3:3 Tainted: G O
V--- 3.10.0-327.36.58.4.x86_64 #1
[ 8981.017667] Workqueue: events linkwatch_event
[ 8981.018652] task: 8800a8348000 ti: 8800b045c000 task.ti:
8800b045c000
[ 8981.020418] RIP: 0010:[] []
__ethtool_get_link_ksettings+0x27/0x140
[ 8981.022383] RSP: 0018:8800b045fc88 EFLAGS: 00010202
[ 8981.023453] RAX: RBX: 8800b045fcac RCX:
[ 8981.024726] RDX: 8800b658f600 RSI: 8800b045fcac RDI: 8802296e
[ 8981.026000] RBP: 8800b045fc98 R08: R09: 0001
[ 8981.027273] R10: 73e0 R11: 082b0cc8adea R12: 8802296e
[ 8981.028561] R13: 8800b566e8c0 R14: 8800b658f600 R15: 8800b566e000
[ 8981.029841] FS: () GS:88023ed8()
knlGS:
[ 8981.031715] CS: 0010 DS: ES: CR0: 80050033
[ 8981.032845] CR2: CR3: b39a9000 CR4: 003407e0
[ 8981.034137] DR0: DR1: DR2:
[ 8981.035427] DR3: DR6: fffe0ff0 DR7: 0400
[ 8981.036702] Stack:
[ 8981.037406] 8800b658f600 9c40 8800b045fce8
a047a71d
[ 8981.039238] 004d 8800b045fcc8 8800b045fd28
815cb198
[ 8981.041070] 8800b045fcd8 810807e6 e8212951
0001
[ 8981.042910] Call Trace:
[ 8981.043660] [] bond_update_speed_duplex+0x3d/0x90
[bonding]
[ 8981.045424] [] ? inetdev_event+0x38/0x530
[ 8981.046554] [] ? put_online_cpus+0x56/0x80
[ 8981.047688] [] bond_netdev_event+0x137/0x360 [bonding]
...
Signed-off-by: Dongxu Liu
---
net/core/ethtool.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 6288e69..9a50b64 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -545,6 +545,8 @@ int __ethtool_get_link_ksettings(struct net_device *dev,
{
ASSERT_RTNL();
+ if (!dev || !dev->ethtool_ops)
+ return -EOPNOTSUPP;
if (!dev->ethtool_ops->get_link_ksettings)
return -EOPNOTSUPP;
--
2.12.3
