[V2] net/ieee802154: fix uninit value bug in dgram_sendmsg

Commit Message

Haimin Zhang Aug. 30, 2022, 7:03 a.m. UTC

There is uninit value bug in dgram_sendmsg function in
net/ieee802154/socket.c when the length of valid data pointed by the
msg->msg_name isn't verified.

We should check the msg_namelen is not less than struct 
sockaddr_ieee802154 when addr_type is SHORT before calling 
ieee802154_addr_from_sa. So we define IEEE802154_MIN_NAMELEN.
And in function ieee802154_addr_from_sa, when 
addr_type is LONG, we check msg_namelen is not less than 
sizeof(struct sockaddr_ieee802154). Meanwhile we check in the 
beginning of function dgram_sendmsg.

Also fixed in raw_bind, dgram_bind, dgram_connect.

Signed-off-by: Haimin Zhang <[e-mail]>
---
 include/net/ieee802154_netdev.h |  9 ++++--
 net/ieee802154/socket.c         | 52 ++++++++++++++++++++-------------
 2 files changed, 39 insertions(+), 22 deletions(-)

Comments

Alexander Aring Sept. 8, 2022, 2:02 a.m. UTC | #1

Hi,

On Tue, Aug 30, 2022 at 3:03 AM Haimin Zhang <[e-mail]> wrote:
>
> There is uninit value bug in dgram_sendmsg function in
> net/ieee802154/socket.c when the length of valid data pointed by the
> msg->msg_name isn't verified.
>
> We should check the msg_namelen is not less than struct
> sockaddr_ieee802154 when addr_type is SHORT before calling
> ieee802154_addr_from_sa. So we define IEEE802154_MIN_NAMELEN.
> And in function ieee802154_addr_from_sa, when
> addr_type is LONG, we check msg_namelen is not less than
> sizeof(struct sockaddr_ieee802154). Meanwhile we check in the
> beginning of function dgram_sendmsg.
>

There exists also an IEEE802154_ADDR_NONE addr_type.

We need to first check that space is there to evaluate the addr_type.

If it's NONE, ignore hwaddr or short address. If it's SHORT or hwaddr
check if they have space for it, if it's something completely
different return -EINVAL.

There are still missing bits and I would recommend introducing a
helper function to do this "kind" of more complex check. This patch
spreads different checks around by checking on IEEE802154_MIN_NAMELEN
(which isn't correct, because NONE) and then requires another check by
calling ieee802154_addr_from_sa() and checking the return code.

- Alex

Patch

diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
index d0d188c32..e1dc3fb02 100644
--- a/include/net/ieee802154_netdev.h
+++ b/include/net/ieee802154_netdev.h
@@ -165,8 +165,8 @@  static inline void ieee802154_devaddr_to_raw(void *raw, __le64 addr)
 	memcpy(raw, &temp, IEEE802154_ADDR_LEN);
 }
 
-static inline void ieee802154_addr_from_sa(struct ieee802154_addr *a,
-					   const struct ieee802154_addr_sa *sa)
+static inline int ieee802154_addr_from_sa(struct ieee802154_addr *a,
+					   const struct ieee802154_addr_sa *sa, int len)
 {
 	a->mode = sa->addr_type;
 	a->pan_id = cpu_to_le16(sa->pan_id);
@@ -176,9 +176,14 @@  static inline void ieee802154_addr_from_sa(struct ieee802154_addr *a,
 		a->short_addr = cpu_to_le16(sa->short_addr);
 		break;
 	case IEEE802154_ADDR_LONG:
+		if (len > sizeof(struct sockaddr_ieee802154))
+			return -EINVAL;
 		a->extended_addr = ieee802154_devaddr_from_raw(sa->hwaddr);
 		break;
+	default:
+		return -EINVAL;
 	}
+	return 0;
 }
 
 static inline void ieee802154_addr_to_sa(struct ieee802154_addr_sa *sa,
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index 718fb77bb..4452aaf68 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -27,6 +27,10 @@ 
 #include <net/af_ieee802154.h>
 #include <net/ieee802154_netdev.h>
 
+#define IEEE802154_MIN_NAMELEN \
+	(offsetof(struct sockaddr_ieee802154, addr) + \
+	 offsetofend(struct ieee802154_addr_sa, short_addr))
+
 /* Utility function for families */
 static struct net_device*
 ieee802154_get_dev(struct net *net, const struct ieee802154_addr *addr)
@@ -200,7 +204,7 @@  static int raw_bind(struct sock *sk, struct sockaddr *_uaddr, int len)
 	int err = 0;
 	struct net_device *dev = NULL;
 
-	if (len < sizeof(*uaddr))
+	if (len < IEEE802154_MIN_NAMELEN)
 		return -EINVAL;
 
 	uaddr = (struct sockaddr_ieee802154 *)_uaddr;
@@ -209,7 +213,9 @@  static int raw_bind(struct sock *sk, struct sockaddr *_uaddr, int len)
 
 	lock_sock(sk);
 
-	ieee802154_addr_from_sa(&addr, &uaddr->addr);
+	err = ieee802154_addr_from_sa(&addr, &uaddr->addr, len);
+	if (err < 0)
+		goto out;
 	dev = ieee802154_get_dev(sock_net(sk), &addr);
 	if (!dev) {
 		err = -ENODEV;
@@ -493,13 +499,15 @@  static int dgram_bind(struct sock *sk, struct sockaddr *uaddr, int len)
 
 	ro->bound = 0;
 
-	if (len < sizeof(*addr))
+	if (len < IEEE802154_MIN_NAMELEN)
 		goto out;
 
 	if (addr->family != AF_IEEE802154)
 		goto out;
 
-	ieee802154_addr_from_sa(&haddr, &addr->addr);
+	err = ieee802154_addr_from_sa(&haddr, &addr->addr, len);
+	if (err < 0)
+		goto out;
 	dev = ieee802154_get_dev(sock_net(sk), &haddr);
 	if (!dev) {
 		err = -ENODEV;
@@ -564,7 +572,7 @@  static int dgram_connect(struct sock *sk, struct sockaddr *uaddr,
 	struct dgram_sock *ro = dgram_sk(sk);
 	int err = 0;
 
-	if (len < sizeof(*addr))
+	if (len < IEEE802154_MIN_NAMELEN)
 		return -EINVAL;
 
 	if (addr->family != AF_IEEE802154)
@@ -577,7 +585,9 @@  static int dgram_connect(struct sock *sk, struct sockaddr *uaddr,
 		goto out;
 	}
 
-	ieee802154_addr_from_sa(&ro->dst_addr, &addr->addr);
+	err = ieee802154_addr_from_sa(&ro->dst_addr, &addr->addr, len);
+	if (err < 0)
+		goto out;
 	ro->connected = 1;
 
 out:
@@ -612,10 +622,22 @@  static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 		return -EOPNOTSUPP;
 	}
 
-	if (!ro->connected && !msg->msg_name)
-		return -EDESTADDRREQ;
-	else if (ro->connected && msg->msg_name)
-		return -EISCONN;
+	if (msg->msg_name) {
+		if (ro->connected)
+			return -EISCONN;
+		if (msg->msg_namelen < IEEE802154_MIN_NAMELEN)
+			return -EINVAL;
+		DECLARE_SOCKADDR(struct sockaddr_ieee802154*,
+				 daddr, msg->msg_name);
+		err = ieee802154_addr_from_sa(&dst_addr, &daddr->addr,
+					      msg->msg_namelen);
+		if (err < 0)
+			return err;
+	} else {
+		if (!ro->connected)
+			return -EDESTADDRREQ;
+		dst_addr = ro->dst_addr;
+	}
 
 	if (!ro->bound)
 		dev = dev_getfirstbyhwtype(sock_net(sk), ARPHRD_IEEE802154);
@@ -651,16 +673,6 @@  static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	cb = mac_cb_init(skb);
 	cb->type = IEEE802154_FC_TYPE_DATA;
 	cb->ackreq = ro->want_ack;
-
-	if (msg->msg_name) {
-		DECLARE_SOCKADDR(struct sockaddr_ieee802154*,
-				 daddr, msg->msg_name);
-
-		ieee802154_addr_from_sa(&dst_addr, &daddr->addr);
-	} else {
-		dst_addr = ro->dst_addr;
-	}
-
 	cb->secen = ro->secen;
 	cb->secen_override = ro->secen_override;
 	cb->seclevel = ro->seclevel;