Skip to contentSkip to conversation

libertas: Avoid reading past end of buffer

PATCH In review Kees Cook 2 participants 3 messages May 9, 2017 – May 10, 2017

Subsystems (3)
Networking Network drivers Marvell libertas wireless driver
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 libertas: Avoid reading past end of buffer

Using memcpy() from a string that is shorter than the length copied means the destination buffer is being filled with arbitrary data from the kernel rodata segment. Instead, use strncpy() which will fill the trailing bytes with zeros. Additionally adjust indentation to keep checkpatch.pl happy.

This was found with the future CONFIG_FORTIFY_SOURCE feature.

Cc Daniel Micay <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 drivers/net/wireless/marvell/libertas/mesh.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/net/wireless/marvell/libertas/mesh.c +3 -3
index d0c881dd5846..d0b1948ca242 100644
@@ -1177,9 +1177,9 @@ void lbs_mesh_ethtool_get_strings(struct net_device *dev,
 	switch (stringset) {
 	case ETH_SS_STATS:
 		for (i = 0; i < MESH_STATS_NUM; i++) {
-			memcpy(s + i * ETH_GSTRING_LEN,
-					mesh_stat_strings[i],
-					ETH_GSTRING_LEN);
+			strncpy(s + i * ETH_GSTRING_LEN,
+				mesh_stat_strings[i],
+				ETH_GSTRING_LEN);
 		}
 		break;
 	}
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
#1 lore raw
Quoted 6 lines · Kees Cook

Using memcpy() from a string that is shorter than the length copied means the destination buffer is being filled with arbitrary data from the kernel rodata segment. Instead, use strncpy() which will fill the trailing bytes with zeros. Additionally adjust indentation to keep checkpatch.pl happy.

This was found with the future CONFIG_FORTIFY_SOURCE feature.

[]

drivers/net/wireless/marvell/libertas/mesh.c

Quoted 1 line
drivers/net/wireless/marvell/libertas/mesh.c +0 -0

[]

drivers/net/wireless/marvell/libertas/mesh.c void lbs_mesh_ethtool_get_strings(struct net_device *dev, · line 1183

Quoted 11 lines
drivers/net/wireless/marvell/libertas/mesh.c +3 -3
@@ -1177,9 +1177,9 @@ void lbs_mesh_ethtool_get_strings(struct net_device *dev,
 	switch (stringset) {
 	case ETH_SS_STATS:
 		for (i = 0; i < MESH_STATS_NUM; i++) {
-			memcpy(s + i * ETH_GSTRING_LEN,
-					mesh_stat_strings[i],
-					ETH_GSTRING_LEN);
+			strncpy(s + i * ETH_GSTRING_LEN,
+				mesh_stat_strings[i],
+				ETH_GSTRING_LEN);
 		}

The better solution is to declare
mesh_stat_strings in in the normal way

---
 drivers/net/wireless/marvell/libertas/mesh.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/net/wireless/marvell/libertas/mesh.c +9 -9
index d0c881dd5846..a535e7f48d2d 100644
@@ -1108,15 +1108,15 @@ void lbs_mesh_set_txpd(struct lbs_private *priv,
  * Ethtool related
  */
 
-static const char * const mesh_stat_strings[] = {
-			"drop_duplicate_bcast",
-			"drop_ttl_zero",
-			"drop_no_fwd_route",
-			"drop_no_buffers",
-			"fwded_unicast_cnt",
-			"fwded_bcast_cnt",
-			"drop_blind_table",
-			"tx_failed_cnt"
+static const char mesh_stat_strings[][ETH_GSTRING_LEN] = {
+	"drop_duplicate_bcast",
+	"drop_ttl_zero",
+	"drop_no_fwd_route",
+	"drop_no_buffers",
+	"fwded_unicast_cnt",
+	"fwded_bcast_cnt",
+	"drop_blind_table",
+	"tx_failed_cnt",
 };
 
 void lbs_mesh_ethtool_get_stats(struct net_device *dev,
author
#2 lore raw
Quoted 48 lines · Joe Perches
Quoted 6 lines · Kees Cook

Using memcpy() from a string that is shorter than the length copied means the destination buffer is being filled with arbitrary data from the kernel rodata segment. Instead, use strncpy() which will fill the trailing bytes with zeros. Additionally adjust indentation to keep checkpatch.pl happy.

This was found with the future CONFIG_FORTIFY_SOURCE feature.

[]

drivers/net/wireless/marvell/libertas/mesh.c

Quoted 1 line
drivers/net/wireless/marvell/libertas/mesh.c +0 -0

[]

drivers/net/wireless/marvell/libertas/mesh.c void lbs_mesh_ethtool_get_strings(struct net_device *dev, · line 1183

Quoted 11 lines
drivers/net/wireless/marvell/libertas/mesh.c +3 -3
@@ -1177,9 +1177,9 @@ void lbs_mesh_ethtool_get_strings(struct net_device *dev,
      switch (stringset) {
      case ETH_SS_STATS:
              for (i = 0; i < MESH_STATS_NUM; i++) {
-                     memcpy(s + i * ETH_GSTRING_LEN,
-                                     mesh_stat_strings[i],
-                                     ETH_GSTRING_LEN);
+                     strncpy(s + i * ETH_GSTRING_LEN,
+                             mesh_stat_strings[i],
+                             ETH_GSTRING_LEN);
              }

The better solution is to declare
mesh_stat_strings in in the normal way

---
 drivers/net/wireless/marvell/libertas/mesh.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/net/wireless/marvell/libertas/mesh.c +2 -9
index d0c881dd5846..a535e7f48d2d 100644
@@ -1108,15 +1108,15 @@ void lbs_mesh_set_txpd(struct lbs_private *priv,
  * Ethtool related
  */
 
-static const char * const mesh_stat_strings[] = {
-                       "drop_duplicate_bcast",
-                       "drop_ttl_zero",
-                       "drop_no_fwd_route",
-                       "drop_no_buffers",
-                       "fwded_unicast_cnt",
-                       "fwded_bcast_cnt",
-                       "drop_blind_table",
-                       "tx_failed_cnt"
+static const char mesh_stat_strings[][ETH_GSTRING_LEN] = {
+       "drop_duplicate_bcast",

drivers/net/wireless/marvell/libertas/mesh.c void lbs_mesh_set_txpd(struct lbs_private *priv, · line 1122

Quoted 10 lines
drivers/net/wireless/marvell/libertas/mesh.c +7 -0
+       "drop_ttl_zero",
+       "drop_no_fwd_route",
+       "drop_no_buffers",
+       "fwded_unicast_cnt",
+       "fwded_bcast_cnt",
+       "drop_blind_table",
+       "tx_failed_cnt",
 };
 
 void lbs_mesh_ethtool_get_stats(struct net_device *dev,

Ah yeah! And that simplifies the memcpy too, since it can just do it in one go. New patch on the way...

-Kees

Signature · 2 lines
Kees Cook
Pixel Security