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 <[email protected]>
---
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
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <[email protected]>:
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
Hi,
just something that we should fix in the future...
On Thu, Sep 8, 2022 at 8:19 AM Haimin Zhang <[email protected]> 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 <[email protected]>
> ---
> 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