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 &default_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 &default_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