Skip to contentSkip to conversation

ethtool: check the ethtool_ops is NULL in dev_ethtool

PATCH Superseded Wang Weidong 2 participants 7 messages Feb 17, 2014 – Feb 18, 2014

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Wang Weidong

author
#0 lore raw

PATCH 1/1 ethtool: check the ethtool_ops is NULL in dev_ethtool

some drivers maybe not implement the ethtool_ops with only set NULL. So when call the ethtool cmds will lead to a 'NULL pointer dereference'.

So add a checking in dev_ethtool.

Signed-off-by Wang Weidong <[email protected]>
---
 net/core/ethtool.c | 3 +++
 1 file changed, 3 insertions(+)
net/core/ethtool.c +3 -0
index 30071de..f418dcb 100644
@@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 	}
+	
+	if (!dev->ethtool_ops)
+		return -EOPNOTSUPP;
 
 	if (dev->ethtool_ops->begin) {
 		rc = dev->ethtool_ops->begin(dev);
Signature · 1 line
1.7.12
#1 lore raw
Quoted 16 lines · Wang Weidong

some drivers maybe not implement the ethtool_ops with only set NULL. So when call the ethtool cmds will lead to a 'NULL pointer dereference'.

So add a checking in dev_ethtool.

Signed-off-by Wang Weidong <[email protected]>
---
  net/core/ethtool.c | 3 +++
  1 file changed, 3 insertions(+)
net/core/ethtool.c +0 -0
index 30071de..f418dcb 100644

net/core/ethtool.c int dev_ethtool(struct net *net, struct ifreq *ifr) · line 1502

Quoted 4 lines
net/core/ethtool.c +1 -0
@@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
  		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
  			return -EPERM;
  	}
+

You have a trailing whitespace/tab in the line above. Please use checkpatch for detecting such things.

Can you be more specific with "some drivers"? Any driver that is in the mainline tree?

Quoted 6 lines
net/core/ethtool.c +2 -0
+	if (!dev->ethtool_ops)
+		return -EOPNOTSUPP;
 
  	if (dev->ethtool_ops->begin) {
  		rc = dev->ethtool_ops->begin(dev);
author
#2 lore raw
Quoted 12 lines · Daniel Borkmann
Quoted 11 lines · Wang Weidong

some drivers maybe not implement the ethtool_ops with only set NULL. So when call the ethtool cmds will lead to a 'NULL pointer dereference'.

So add a checking in dev_ethtool.

Signed-off-by Wang Weidong <[email protected]>
---
  net/core/ethtool.c | 3 +++
  1 file changed, 3 insertions(+)
Quoted 13 lines

net/core/ethtool.c int dev_ethtool(struct net *net, struct ifreq *ifr) · line 1502

Quoted 9 lines
net/core/ethtool.c +1 -0
index 30071de..f418dcb 100644
@@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
          if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
              return -EPERM;
      }
+

You have a trailing whitespace/tab in the line above. Please use checkpatch for detecting such things.

Sorry for that. I will fix it soon.

Quoted 3 lines

Can you be more specific with "some drivers"? Any driver that is in the mainline tree?

No. My team implements a driver without considering the ethtool_ops. So I got the problem.

Quoted 8 lines
Quoted 6 lines
net/core/ethtool.c +2 -0
+    if (!dev->ethtool_ops)
+        return -EOPNOTSUPP;
 
      if (dev->ethtool_ops->begin) {
          rc = dev->ethtool_ops->begin(dev);
#3 lore raw
Quoted 26 lines · Wang Weidong
Quoted 12 lines · Daniel Borkmann
Quoted 11 lines · Wang Weidong

some drivers maybe not implement the ethtool_ops with only set NULL. So when call the ethtool cmds will lead to a 'NULL pointer dereference'.

So add a checking in dev_ethtool.

Signed-off-by Wang Weidong <[email protected]>
---
   net/core/ethtool.c | 3 +++
   1 file changed, 3 insertions(+)
Quoted 13 lines

net/core/ethtool.c int dev_ethtool(struct net *net, struct ifreq *ifr) · line 1502

Quoted 9 lines
net/core/ethtool.c +1 -0
index 30071de..f418dcb 100644
@@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
           if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
               return -EPERM;
       }
+

You have a trailing whitespace/tab in the line above. Please use checkpatch for detecting such things.

Quoted 7 lines

Sorry for that. I will fix it soon.

Quoted 3 lines

Can you be more specific with "some drivers"? Any driver that is in the mainline tree?

No. My team implements a driver without considering the ethtool_ops. So I got the problem.

If it's code that is out of the mainline tree, then why should the kernel support that? Submit your driver to the tree first, and then such a change could be considered. Or, even better, let them implement ethtool ops/stubs.

Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
net/core/ethtool.c +2 -0
+    if (!dev->ethtool_ops)
+        return -EOPNOTSUPP;
 
       if (dev->ethtool_ops->begin) {
           rc = dev->ethtool_ops->begin(dev);
author
#4 lore raw
Quoted 32 lines · Daniel Borkmann
Quoted 31 lines · Wang Weidong
Quoted 12 lines · Daniel Borkmann
Quoted 11 lines · Wang Weidong

some drivers maybe not implement the ethtool_ops with only set NULL. So when call the ethtool cmds will lead to a 'NULL pointer dereference'.

So add a checking in dev_ethtool.

Signed-off-by Wang Weidong <[email protected]>
---
   net/core/ethtool.c | 3 +++
   1 file changed, 3 insertions(+)
Quoted 13 lines

net/core/ethtool.c int dev_ethtool(struct net *net, struct ifreq *ifr) · line 1502

Quoted 9 lines
net/core/ethtool.c +1 -0
index 30071de..f418dcb 100644
@@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
           if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
               return -EPERM;
       }
+

You have a trailing whitespace/tab in the line above. Please use checkpatch for detecting such things.

Sorry for that. I will fix it soon.

Quoted 3 lines

Can you be more specific with "some drivers"? Any driver that is in the mainline tree?

Quoted 8 lines
Quoted 2 lines

No. My team implements a driver without considering the ethtool_ops. So I got the problem.

If it's code that is out of the mainline tree, then why should the kernel support that? Submit your driver to the tree first, and then such a change could be considered. Or, even better, let them implement ethtool ops/stubs.

Thanks for your reply. I will suggest them to implement ethtool ops.

Regards
Wang

Quoted 12 lines
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
net/core/ethtool.c +2 -0
+    if (!dev->ethtool_ops)
+        return -EOPNOTSUPP;
 
       if (dev->ethtool_ops->begin) {
           rc = dev->ethtool_ops->begin(dev);
author
#5 lore raw

PATCH v2 1/1 ethtool: check the ethtool_ops is NULL in dev_ethtool

In reply to Wang Weidong

some drivers maybe not implement the ethtool_ops with only set NULL to ethtool_ops. So when call the ethtool devx will lead to a 'NULL pointer dereference'.

So add a check in dev_ethtool

Signed-off-by Wang Weidong <[email protected]>

---
Change note:

v2: fix a trailing whitespace/tab pointed out by Daniel

---
 net/core/ethtool.c | 3 +++
 1 file changed, 3 insertions(+)
net/core/ethtool.c +3 -0
index 30071de..c8cfd8f 100644
@@ -1500,6 +1500,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 			return -EPERM;
 	}
 
+	if (!dev->ethtool_ops)
+		return -EOPNOTSUPP;
+
 	if (dev->ethtool_ops->begin) {
 		rc = dev->ethtool_ops->begin(dev);
 		if (rc  < 0)
Signature · 1 line
1.7.12
author
#6 lore raw

Just ignore it.

Regards
Wang

Quoted 31 lines · Wang Weidong

some drivers maybe not implement the ethtool_ops with only set NULL to ethtool_ops. So when call the ethtool devx will lead to a 'NULL pointer dereference'.

So add a check in dev_ethtool

Signed-off-by Wang Weidong <[email protected]>

---
Change note:

v2: fix a trailing whitespace/tab pointed out by Daniel

---
 net/core/ethtool.c | 3 +++
 1 file changed, 3 insertions(+)
net/core/ethtool.c +3 -0
index 30071de..c8cfd8f 100644
@@ -1500,6 +1500,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 			return -EPERM;
 	}
 
+	if (!dev->ethtool_ops)
+		return -EOPNOTSUPP;
+
 	if (dev->ethtool_ops->begin) {
 		rc = dev->ethtool_ops->begin(dev);
 		if (rc  < 0)