[PATCH] ethtool: fix a kernel infoleak in ethtool_get_pauseparam

Subsystems: networking [general], the rest

STALE3745d

3 messages, 3 authors, 2016-06-01 · open the first message on its own page

[PATCH] ethtool: fix a kernel infoleak in ethtool_get_pauseparam

From: Kangjie Lu <hidden>
Date: 2016-06-01 14:39:34 

The field autoneg of pauseparam is not initialized in some
implementations of get_pauseparam(), but the whole object is
copied to userland.

Signed-off-by: Kangjie Lu <redacted>
---
 net/core/ethtool.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f426c5a..84544bd 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1723,7 +1723,10 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 
 static int ethtool_get_pauseparam(struct net_device *dev, void __user *useraddr)
 {
-	struct ethtool_pauseparam pauseparam = { ETHTOOL_GPAUSEPARAM };
+	struct ethtool_pauseparam pauseparam;
+
+	memset(&pauseparam, 0, sizeof(pauseparam));
+	pauseparam.cmd = ETHTOOL_GPAUSEPARAM;
 
 	if (!dev->ethtool_ops->get_pauseparam)
 		return -EOPNOTSUPP;
-- 
1.9.1

Re: [PATCH] ethtool: fix a kernel infoleak in ethtool_get_pauseparam

From: Edward Cree <hidden>
Date: 2016-06-01 15:05:57 

On 01/06/16 15:39, Kangjie Lu wrote:
quoted hunk
The field autoneg of pauseparam is not initialized in some
implementations of get_pauseparam(), but the whole object is
copied to userland.

Signed-off-by: Kangjie Lu <redacted>
---
 net/core/ethtool.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f426c5a..84544bd 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1723,7 +1723,10 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 
 static int ethtool_get_pauseparam(struct net_device *dev, void __user *useraddr)
 {
-	struct ethtool_pauseparam pauseparam = { ETHTOOL_GPAUSEPARAM };
AIUI an incomplete compound initialiser will fill all unspecified fields
with zeroes of the appropriate type.  So this patch is unnecessary.

Per C99, §6.7.8.21:
If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate [...] the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.
-Ed

+	struct ethtool_pauseparam pauseparam;
+
+	memset(&pauseparam, 0, sizeof(pauseparam));
+	pauseparam.cmd = ETHTOOL_GPAUSEPARAM;
 
 	if (!dev->ethtool_ops->get_pauseparam)
 		return -EOPNOTSUPP;

Re: [PATCH] ethtool: fix a kernel infoleak in ethtool_get_pauseparam

From: Ben Hutchings <hidden>
Date: 2016-06-01 17:14:44 

On Wed, 2016-06-01 at 16:39 +0200, Kangjie Lu wrote:
The field autoneg of pauseparam is not initialized in some
implementations of get_pauseparam(),
Nonsense.  The current implementation initialises all fields.  (If
there was padding in the structure, this change would be needed to
guarantee that the padding was initialised.  But there isn't.)

Ben.
quoted hunk
 but the whole object is
copied to userland.

Signed-off-by: Kangjie Lu <redacted>
---
 net/core/ethtool.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f426c5a..84544bd 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1723,7 +1723,10 @@ static noinline_for_stack int
ethtool_set_channels(struct net_device *dev,
 
 static int ethtool_get_pauseparam(struct net_device *dev, void
__user *useraddr)
 {
-	struct ethtool_pauseparam pauseparam = { ETHTOOL_GPAUSEPARAM
};
+	struct ethtool_pauseparam pauseparam;
+
+	memset(&pauseparam, 0, sizeof(pauseparam));
+	pauseparam.cmd = ETHTOOL_GPAUSEPARAM;
 
 	if (!dev->ethtool_ops->get_pauseparam)
 		return -EOPNOTSUPP;
-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help