Skip to contentSkip to conversation

lib/net_utils: Relax NULL-termination requirement on input string

PATCH In review Andy Shevchenko 3 participants 6 messages Dec 19, 2017 – Dec 20, 2017

Subsystems (4)
Networking Network drivers Staging Library code
lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

PATCH 1/4 lib/net_utils: Relax NULL-termination requirement on input string

We have not been caring about garbage at the end of input string and this change doesn't affect it. What we prevent here is possibility to go over boundaries when input string is not NULL terminated at all.

Allow mac_pton() work on non-NULL terminated strings.

Signed-off-by Andy Shevchenko <[email protected]>
---
 lib/net_utils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
lib/net_utils.c +3 -1
index af525353395d..d32c6961fe0f 100644
@@ -4,12 +4,14 @@
 #include <linux/ctype.h>
 #include <linux/kernel.h>
 
+#define MAC_PTON_MINLEN		(3 * ETH_ALEN - 1)
+
 bool mac_pton(const char *s, u8 *mac)
 {
 	int i;
 
 	/* XX:XX:XX:XX:XX:XX */
-	if (strlen(s) < 3 * ETH_ALEN - 1)
+	if (strnlen(s, MAC_PTON_MINLEN) < MAC_PTON_MINLEN)
 		return false;
 
 	/* Don't dirty result unless string is valid MAC. */
Signature · 1 line
2.15.1
author
#1 lore raw

PATCH 2/4 lib/net_utils: Introduce mac_pton_from_user()

Some drivers are getting MAC from user space. Make a helper for them.

Signed-off-by Andy Shevchenko <[email protected]>
---
 include/linux/kernel.h |  1 +
 lib/net_utils.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+)
include/linux/kernel.h +1 -0
index ce51455e2adf..e203b313608d 100644
@@ -587,6 +587,7 @@ extern int __must_check hex2bin(u8 *dst, const char *src, size_t count);
 extern char *bin2hex(char *dst, const void *src, size_t count);
 
 bool mac_pton(const char *s, u8 *mac);
+int __must_check mac_pton_from_user(const char __user *s, size_t count, u8 *mac);
 
 /*
  * General tracing related utility functions - trace_printk(),
lib/net_utils.c +12 -0
index d32c6961fe0f..7be3483aece6 100644
@@ -3,6 +3,7 @@
 #include <linux/if_ether.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/uaccess.h>
 
 #define MAC_PTON_MINLEN		(3 * ETH_ALEN - 1)
 

@@ -27,3 +28,14 @@ bool mac_pton(const char *s, u8 *mac)
 	return true;
 }
 EXPORT_SYMBOL(mac_pton);
+
+int mac_pton_from_user(const char __user *s, size_t count, u8 *mac)
+{
+	char buf[MAC_PTON_MINLEN];
+
+	count = min(count, sizeof(buf));
+	if (copy_from_user(buf, s, count))
+		return -EFAULT;
+	return mac_pton(buf, mac) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL(mac_pton_from_user);
Signature · 1 line
2.15.1
maintainer
#2 lore raw

Re: [PATCH v1 2/4] lib/net_utils: Introduce mac_pton_from_user()

Quoted 7 lines · Andy Shevchenko

Some drivers are getting MAC from user space. Make a helper for them.

Signed-off-by Andy Shevchenko <[email protected]>
---
 include/linux/kernel.h |  1 +
 lib/net_utils.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+)

Don't do this just for some horrid staging drivers. They can just drop that functionality entirely and use the "normal" way of doing this if they really want it.

thanks,

greg k-h

maintainer
#3 lore raw

Re: [PATCH v1 2/4] lib/net_utils: Introduce mac_pton_from_user()

From: Greg Kroah-Hartman <[email protected]>
Date: Wed, 20 Dec 2017 08:13:55 +0100

Quoted 9 lines · Andy Shevchenko
Quoted 7 lines

Some drivers are getting MAC from user space. Make a helper for them.

Signed-off-by Andy Shevchenko <[email protected]>
---
 include/linux/kernel.h |  1 +
 lib/net_utils.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+)
Quoted 3 lines

Don't do this just for some horrid staging drivers. They can just drop that functionality entirely and use the "normal" way of doing this if they really want it.

Agreed.

author
#4 lore raw

PATCH 3/4 staging: rtl8723bs: Replace mac address parsing

In reply to Andy Shevchenko

Replace copy_from_user() + sscanf() with mac_pton_from_user().

While here, replace memcpy(..., ETH_ALEN) with ether_addr_copy().

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/staging/rtl8723bs/core/rtw_debug.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)
drivers/staging/rtl8723bs/core/rtw_debug.c +6 -12
index b5dd244fee8f..79ac91a6c3ba 100644
@@ -380,21 +380,15 @@ ssize_t proc_set_roam_tgt_addr(struct file *file, const char __user *buffer, siz
 {
 	struct net_device *dev = data;
 	struct adapter *adapter = (struct adapter *)rtw_netdev_priv(dev);
-
-	char tmp[32];
 	u8 addr[ETH_ALEN];
+	int ret;
 
-	if (count < 1)
-		return -EFAULT;
-
-	if (buffer && !copy_from_user(tmp, buffer, sizeof(tmp))) {
-
-		int num = sscanf(tmp, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx", addr, addr+1, addr+2, addr+3, addr+4, addr+5);
-		if (num == 6)
-			memcpy(adapter->mlmepriv.roam_tgt_addr, addr, ETH_ALEN);
+	ret = mac_pton_from_user(buffer, count, addr);
+	if (ret)
+		return ret;
 
-		DBG_871X("set roam_tgt_addr to "MAC_FMT"\n", MAC_ARG(adapter->mlmepriv.roam_tgt_addr));
-	}
+	ether_addr_copy(adapter->mlmepriv.roam_tgt_addr, addr);
+	DBG_871X("set roam_tgt_addr to "MAC_FMT"\n", MAC_ARG(adapter->mlmepriv.roam_tgt_addr));
 
 	return count;
 }
Signature · 1 line
2.15.1
author
#5 lore raw

PATCH 4/4 staging: rtl8712: Replace mac address parsing

In reply to Andy Shevchenko

Replace copy_from_user() + mac_pton() with mac_pton_from_user().

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +6 -12
index e30a5be5f318..3f82c39eb071 100644
@@ -1953,7 +1953,7 @@ static int r871x_get_ap_info(struct net_device *dev,
 	struct list_head *plist, *phead;
 	unsigned char *pbuf;
 	u8 bssid[ETH_ALEN];
-	char data[33];
+	int ret;
 
 	if (padapter->bDriverStopped || (pdata == NULL))
 		return -EINVAL;

@@ -1967,9 +1967,11 @@ static int r871x_get_ap_info(struct net_device *dev,
 	pdata->flags = 0;
 	if (pdata->length < 32)
 		return -EINVAL;
-	if (copy_from_user(data, pdata->pointer, 32))
-		return -EINVAL;
-	data[32] = 0;
+	ret = mac_pton_from_user(pdata->pointer, 32, bssid);
+	if (ret)
+		return ret;
+
+	netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
 
 	spin_lock_irqsave(&(pmlmepriv->scanned_queue.lock), irqL);
 	phead = &queue->queue;

@@ -1978,14 +1980,6 @@ static int r871x_get_ap_info(struct net_device *dev,
 		if (end_of_queue_search(phead, plist))
 			break;
 		pnetwork = container_of(plist, struct wlan_network, list);
-		if (!mac_pton(data, bssid)) {
-			netdev_info(dev, "r8712u: Invalid BSSID '%s'.\n",
-				    (u8 *)data);
-			spin_unlock_irqrestore(&(pmlmepriv->scanned_queue.lock),
-					       irqL);
-			return -EINVAL;
-		}
-		netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
 		if (ether_addr_equal(bssid, pnetwork->network.MacAddress)) {
 			/* BSSID match, then check if supporting wpa/wpa2 */
 			pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
Signature · 1 line
2.15.1