All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V3] net/ieee802154: fix uninit value bug in dgram_sendmsg
@ 2022-09-08 12:19 Haimin Zhang
  2022-09-16 10:00 ` patchwork-bot+netdevbpf
  2022-09-16 11:13 ` Alexander Aring
  0 siblings, 2 replies; 3+ messages in thread
From: Haimin Zhang @ 2022-09-08 12:19 UTC (permalink / raw)
  To: alex.aring, stefan, davem, edumazet, kuba, pabeni, linux-wpan,
	netdev, linux-kernel
  Cc: Haimin Zhang

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 introducing a helper function ieee802154_sockaddr_check_size to 
check namelen. First we check there is addr_type in ieee802154_addr_sa.
Then, we check namelen according to addr_type.

Also fixed in raw_bind, dgram_bind, dgram_connect.

Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>
---
 include/net/ieee802154_netdev.h | 37 +++++++++++++++++++++++++++++
 net/ieee802154/socket.c         | 42 ++++++++++++++++++---------------
 2 files changed, 60 insertions(+), 19 deletions(-)

diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
index d0d188c32..a8994f307 100644
--- a/include/net/ieee802154_netdev.h
+++ b/include/net/ieee802154_netdev.h
@@ -15,6 +15,22 @@
 #ifndef IEEE802154_NETDEVICE_H
 #define IEEE802154_NETDEVICE_H
 
+#define IEEE802154_REQUIRED_SIZE(struct_type, member) \
+	(offsetof(typeof(struct_type), member) + \
+	sizeof(((typeof(struct_type) *)(NULL))->member))
+
+#define IEEE802154_ADDR_OFFSET \
+	offsetof(typeof(struct sockaddr_ieee802154), addr)
+
+#define IEEE802154_MIN_NAMELEN (IEEE802154_ADDR_OFFSET + \
+	IEEE802154_REQUIRED_SIZE(struct ieee802154_addr_sa, addr_type))
+
+#define IEEE802154_NAMELEN_SHORT (IEEE802154_ADDR_OFFSET + \
+	IEEE802154_REQUIRED_SIZE(struct ieee802154_addr_sa, short_addr))
+
+#define IEEE802154_NAMELEN_LONG (IEEE802154_ADDR_OFFSET + \
+	IEEE802154_REQUIRED_SIZE(struct ieee802154_addr_sa, hwaddr))
+
 #include <net/af_ieee802154.h>
 #include <linux/netdevice.h>
 #include <linux/skbuff.h>
@@ -165,6 +181,27 @@ static inline void ieee802154_devaddr_to_raw(void *raw, __le64 addr)
 	memcpy(raw, &temp, IEEE802154_ADDR_LEN);
 }
 
+static inline int
+ieee802154_sockaddr_check_size(struct sockaddr_ieee802154 *daddr, int len)
+{
+	struct ieee802154_addr_sa *sa;
+
+	sa = &daddr->addr;
+	if (len < IEEE802154_MIN_NAMELEN)
+		return -EINVAL;
+	switch (sa->addr_type) {
+	case IEEE802154_ADDR_SHORT:
+		if (len < IEEE802154_NAMELEN_SHORT)
+			return -EINVAL;
+		break;
+	case IEEE802154_ADDR_LONG:
+		if (len < IEEE802154_NAMELEN_LONG)
+			return -EINVAL;
+		break;
+	}
+	return 0;
+}
+
 static inline void ieee802154_addr_from_sa(struct ieee802154_addr *a,
 					   const struct ieee802154_addr_sa *sa)
 {
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index 718fb77bb..7889e1ef7 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -200,8 +200,9 @@ static int raw_bind(struct sock *sk, struct sockaddr *_uaddr, int len)
 	int err = 0;
 	struct net_device *dev = NULL;
 
-	if (len < sizeof(*uaddr))
-		return -EINVAL;
+	err = ieee802154_sockaddr_check_size(uaddr, len);
+	if (err < 0)
+		return err;
 
 	uaddr = (struct sockaddr_ieee802154 *)_uaddr;
 	if (uaddr->family != AF_IEEE802154)
@@ -493,7 +494,8 @@ static int dgram_bind(struct sock *sk, struct sockaddr *uaddr, int len)
 
 	ro->bound = 0;
 
-	if (len < sizeof(*addr))
+	err = ieee802154_sockaddr_check_size(addr, len);
+	if (err < 0)
 		goto out;
 
 	if (addr->family != AF_IEEE802154)
@@ -564,8 +566,9 @@ static int dgram_connect(struct sock *sk, struct sockaddr *uaddr,
 	struct dgram_sock *ro = dgram_sk(sk);
 	int err = 0;
 
-	if (len < sizeof(*addr))
-		return -EINVAL;
+	err = ieee802154_sockaddr_check_size(addr, len);
+	if (err < 0)
+		return err;
 
 	if (addr->family != AF_IEEE802154)
 		return -EINVAL;
@@ -604,6 +607,7 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	struct ieee802154_mac_cb *cb;
 	struct dgram_sock *ro = dgram_sk(sk);
 	struct ieee802154_addr dst_addr;
+	DECLARE_SOCKADDR(struct sockaddr_ieee802154*, daddr, msg->msg_name);
 	int hlen, tlen;
 	int err;
 
@@ -612,10 +616,20 @@ 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;
+		err = ieee802154_sockaddr_check_size(daddr, msg->msg_namelen);
+		if (err < 0)
+			return err;
+		ieee802154_addr_from_sa(&dst_addr, &daddr->addr);
+	} else {
+		if (!ro->connected)
+			return -EDESTADDRREQ;
+		dst_addr = ro->dst_addr;
+	}
 
 	if (!ro->bound)
 		dev = dev_getfirstbyhwtype(sock_net(sk), ARPHRD_IEEE802154);
@@ -651,16 +665,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;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH V3] net/ieee802154: fix uninit value bug in dgram_sendmsg
  2022-09-08 12:19 [PATCH V3] net/ieee802154: fix uninit value bug in dgram_sendmsg Haimin Zhang
@ 2022-09-16 10:00 ` patchwork-bot+netdevbpf
  2022-09-16 11:13 ` Alexander Aring
  1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-09-16 10:00 UTC (permalink / raw)
  To: Haimin Zhang
  Cc: alex.aring, stefan, davem, edumazet, kuba, pabeni, linux-wpan,
	netdev, linux-kernel, tcs_kernel

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu,  8 Sep 2022 20:19:27 +0800 you 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 introducing a helper function ieee802154_sockaddr_check_size to
> check namelen. First we check there is addr_type in ieee802154_addr_sa.
> Then, we check namelen according to addr_type.
> 
> [...]

Here is the summary with links:
  - [V3] net/ieee802154: fix uninit value bug in dgram_sendmsg
    https://git.kernel.org/netdev/net/c/94160108a70c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH V3] net/ieee802154: fix uninit value bug in dgram_sendmsg
  2022-09-08 12:19 [PATCH V3] net/ieee802154: fix uninit value bug in dgram_sendmsg Haimin Zhang
  2022-09-16 10:00 ` patchwork-bot+netdevbpf
@ 2022-09-16 11:13 ` Alexander Aring
  1 sibling, 0 replies; 3+ messages in thread
From: Alexander Aring @ 2022-09-16 11:13 UTC (permalink / raw)
  To: Haimin Zhang
  Cc: alex.aring, stefan, davem, edumazet, kuba, pabeni, linux-wpan,
	netdev, linux-kernel, Haimin Zhang

Hi,

just something that we should fix in the future...

On Thu, Sep 8, 2022 at 8:19 AM Haimin Zhang <tcs.kernel@gmail.com> 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 introducing a helper function ieee802154_sockaddr_check_size to
> check namelen. First we check there is addr_type in ieee802154_addr_sa.
> Then, we check namelen according to addr_type.
>
> Also fixed in raw_bind, dgram_bind, dgram_connect.
>
> Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>
> ---
>  include/net/ieee802154_netdev.h | 37 +++++++++++++++++++++++++++++
>  net/ieee802154/socket.c         | 42 ++++++++++++++++++---------------
>  2 files changed, 60 insertions(+), 19 deletions(-)
>
> diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> index d0d188c32..a8994f307 100644
> --- a/include/net/ieee802154_netdev.h
> +++ b/include/net/ieee802154_netdev.h
> @@ -15,6 +15,22 @@
>  #ifndef IEEE802154_NETDEVICE_H
>  #define IEEE802154_NETDEVICE_H
>
> +#define IEEE802154_REQUIRED_SIZE(struct_type, member) \
> +       (offsetof(typeof(struct_type), member) + \
> +       sizeof(((typeof(struct_type) *)(NULL))->member))
> +
> +#define IEEE802154_ADDR_OFFSET \
> +       offsetof(typeof(struct sockaddr_ieee802154), addr)
> +
> +#define IEEE802154_MIN_NAMELEN (IEEE802154_ADDR_OFFSET + \
> +       IEEE802154_REQUIRED_SIZE(struct ieee802154_addr_sa, addr_type))
> +
> +#define IEEE802154_NAMELEN_SHORT (IEEE802154_ADDR_OFFSET + \
> +       IEEE802154_REQUIRED_SIZE(struct ieee802154_addr_sa, short_addr))
> +
> +#define IEEE802154_NAMELEN_LONG (IEEE802154_ADDR_OFFSET + \
> +       IEEE802154_REQUIRED_SIZE(struct ieee802154_addr_sa, hwaddr))
> +
>  #include <net/af_ieee802154.h>
>  #include <linux/netdevice.h>
>  #include <linux/skbuff.h>
> @@ -165,6 +181,27 @@ static inline void ieee802154_devaddr_to_raw(void *raw, __le64 addr)
>         memcpy(raw, &temp, IEEE802154_ADDR_LEN);
>  }
>
> +static inline int
> +ieee802154_sockaddr_check_size(struct sockaddr_ieee802154 *daddr, int len)
> +{
> +       struct ieee802154_addr_sa *sa;
> +
> +       sa = &daddr->addr;
> +       if (len < IEEE802154_MIN_NAMELEN)
> +               return -EINVAL;
> +       switch (sa->addr_type) {
> +       case IEEE802154_ADDR_SHORT:
> +               if (len < IEEE802154_NAMELEN_SHORT)
> +                       return -EINVAL;
> +               break;
> +       case IEEE802154_ADDR_LONG:
> +               if (len < IEEE802154_NAMELEN_LONG)
> +                       return -EINVAL;
> +               break;
> +       }

There is a missing IEEE802154_ADDR_NONE here. If it's NONE the size is
correct, although all other possible values which are not SHORT, LONG
or NONE, should here end in an -EINVAL as well. In those cases the
size is "undefined" for now.

> +       return 0;
> +}
> +
>  static inline void ieee802154_addr_from_sa(struct ieee802154_addr *a,
>                                            const struct ieee802154_addr_sa *sa)
>  {
> diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> index 718fb77bb..7889e1ef7 100644
> --- a/net/ieee802154/socket.c
> +++ b/net/ieee802154/socket.c
> @@ -200,8 +200,9 @@ static int raw_bind(struct sock *sk, struct sockaddr *_uaddr, int len)
>         int err = 0;
>         struct net_device *dev = NULL;
>
> -       if (len < sizeof(*uaddr))
> -               return -EINVAL;
> +       err = ieee802154_sockaddr_check_size(uaddr, len);
> +       if (err < 0)
> +               return err;
>
>         uaddr = (struct sockaddr_ieee802154 *)_uaddr;
>         if (uaddr->family != AF_IEEE802154)
> @@ -493,7 +494,8 @@ static int dgram_bind(struct sock *sk, struct sockaddr *uaddr, int len)
>
>         ro->bound = 0;
>
> -       if (len < sizeof(*addr))
> +       err = ieee802154_sockaddr_check_size(addr, len);
> +       if (err < 0)
>                 goto out;
>
>         if (addr->family != AF_IEEE802154)
> @@ -564,8 +566,9 @@ static int dgram_connect(struct sock *sk, struct sockaddr *uaddr,
>         struct dgram_sock *ro = dgram_sk(sk);
>         int err = 0;
>
> -       if (len < sizeof(*addr))
> -               return -EINVAL;
> +       err = ieee802154_sockaddr_check_size(addr, len);
> +       if (err < 0)
> +               return err;
>
>         if (addr->family != AF_IEEE802154)
>                 return -EINVAL;
> @@ -604,6 +607,7 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>         struct ieee802154_mac_cb *cb;
>         struct dgram_sock *ro = dgram_sk(sk);
>         struct ieee802154_addr dst_addr;
> +       DECLARE_SOCKADDR(struct sockaddr_ieee802154*, daddr, msg->msg_name);
>         int hlen, tlen;
>         int err;
>
> @@ -612,10 +616,20 @@ 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;

nitpick, we do this in ieee802154_sockaddr_check_size() as well?

> +               err = ieee802154_sockaddr_check_size(daddr, msg->msg_namelen);
> +               if (err < 0)
> +                       return err;
> +               ieee802154_addr_from_sa(&dst_addr, &daddr->addr);
> +       } else {
> +               if (!ro->connected)
> +                       return -EDESTADDRREQ;

I'm not sure about this change but it looks okay to me.

That's it. I am sorry for the delay... I usually schedule my review
for the weekend if I can't do it then the next weekend...

- Alex


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-16 11:13 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-08 12:19 [PATCH V3] net/ieee802154: fix uninit value bug in dgram_sendmsg Haimin Zhang
2022-09-16 10:00 ` patchwork-bot+netdevbpf
2022-09-16 11:13 ` Alexander Aring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.