lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20220901030610.1121299-3-keescook@chromium.org>
Date:   Wed, 31 Aug 2022 20:06:10 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Jozsef Kadlecsik <kadlec@...filter.org>,
        Florian Westphal <fw@...len.de>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        syzbot <syzkaller@...glegroups.com>,
        netfilter-devel@...r.kernel.org, coreteam@...filter.org,
        netdev@...r.kernel.org, Yajun Deng <yajun.deng@...ux.dev>,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: [PATCH 2/2] netlink: Bounds-check struct nlmsgerr creation

For 32-bit systems, it might be possible to wrap lnmsgerr content
lengths beyond SIZE_MAX. Explicitly test for all overflows, and mark the
memcpy() as being unable to internally diagnose overflows.

This also excludes netlink from the coming runtime bounds check on
memcpy(), since it's an unusual case of open-coded sizing and
allocation.

Cc: Pablo Neira Ayuso <pablo@...filter.org>
Cc: Jozsef Kadlecsik <kadlec@...filter.org>
Cc: Florian Westphal <fw@...len.de>
Cc: "David S. Miller" <davem@...emloft.net>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>
Cc: Paolo Abeni <pabeni@...hat.com>
Cc: syzbot <syzkaller@...glegroups.com>
Cc: netfilter-devel@...r.kernel.org
Cc: coreteam@...filter.org
Cc: netdev@...r.kernel.org
Signed-off-by: Kees Cook <keescook@...omium.org>
---
 net/netfilter/ipset/ip_set_core.c | 10 +++++--
 net/netlink/af_netlink.c          | 49 +++++++++++++++++++++----------
 2 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 16ae92054baa..43576f68f53d 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1709,13 +1709,14 @@ call_ad(struct net *net, struct sock *ctnl, struct sk_buff *skb,
 		struct nlmsghdr *rep, *nlh = nlmsg_hdr(skb);
 		struct sk_buff *skb2;
 		struct nlmsgerr *errmsg;
-		size_t payload = min(SIZE_MAX,
-				     sizeof(*errmsg) + nlmsg_len(nlh));
+		size_t payload;
 		int min_len = nlmsg_total_size(sizeof(struct nfgenmsg));
 		struct nlattr *cda[IPSET_ATTR_CMD_MAX + 1];
 		struct nlattr *cmdattr;
 		u32 *errline;
 
+		if (check_add_overflow(sizeof(*errmsg), nlmsg_len(nlh), &payload))
+			return -ENOMEM;
 		skb2 = nlmsg_new(payload, GFP_KERNEL);
 		if (!skb2)
 			return -ENOMEM;
@@ -1723,7 +1724,10 @@ call_ad(struct net *net, struct sock *ctnl, struct sk_buff *skb,
 				  nlh->nlmsg_seq, NLMSG_ERROR, payload, 0);
 		errmsg = nlmsg_data(rep);
 		errmsg->error = ret;
-		memcpy(&errmsg->msg, nlh, nlh->nlmsg_len);
+		unsafe_memcpy(&errmsg->msg, nlh, nlh->nlmsg_len,
+			      /* "payload" was explicitly bounds-checked, based on
+			       * the size of nlh->nlmsg_len.
+			       */);
 		cmdattr = (void *)&errmsg->msg + min_len;
 
 		ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, cmdattr,
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 0cd91f813a3b..8779c273f34f 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2407,7 +2407,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 	struct nlmsghdr *rep;
 	struct nlmsgerr *errmsg;
 	size_t payload = sizeof(*errmsg);
-	size_t tlvlen = 0;
+	size_t alloc_size, tlvlen = 0;
 	struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
 	unsigned int flags = 0;
 	bool nlk_has_extack = nlk->flags & NETLINK_F_EXT_ACK;
@@ -2419,32 +2419,44 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 	if (nlk_has_extack && extack && extack->_msg)
 		tlvlen += nla_total_size(strlen(extack->_msg) + 1);
 
-	if (err && !(nlk->flags & NETLINK_F_CAP_ACK))
-		payload += nlmsg_len(nlh);
+	if (err && !(nlk->flags & NETLINK_F_CAP_ACK) &&
+	    check_add_overflow(payload, (size_t)nlmsg_len(nlh), &payload))
+		goto failure;
 	else
 		flags |= NLM_F_CAPPED;
-	if (err && nlk_has_extack && extack && extack->bad_attr)
-		tlvlen += nla_total_size(sizeof(u32));
-	if (nlk_has_extack && extack && extack->cookie_len)
-		tlvlen += nla_total_size(extack->cookie_len);
-	if (err && nlk_has_extack && extack && extack->policy)
-		tlvlen += netlink_policy_dump_attr_size_estimate(extack->policy);
+	if (err && nlk_has_extack && extack && extack->bad_attr &&
+	    check_add_overflow(tlvlen, (size_t)nla_total_size(sizeof(u32)),
+			       &tlvlen))
+		goto failure;
+	if (nlk_has_extack && extack && extack->cookie_len &&
+	    check_add_overflow(tlvlen, (size_t)nla_total_size(extack->cookie_len),
+			       &tlvlen))
+		goto failure;
+	if (err && nlk_has_extack && extack && extack->policy &&
+	    check_add_overflow(tlvlen,
+			       (size_t)netlink_policy_dump_attr_size_estimate(extack->policy),
+			       &tlvlen))
+		goto failure;
 
 	if (tlvlen)
 		flags |= NLM_F_ACK_TLVS;
 
-	skb = nlmsg_new(payload + tlvlen, GFP_KERNEL);
-	if (!skb) {
-		NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
-		sk_error_report(NETLINK_CB(in_skb).sk);
-		return;
-	}
+	if (check_add_overflow(payload, tlvlen, &alloc_size))
+		goto failure;
+
+	skb = nlmsg_new(alloc_size, GFP_KERNEL);
+	if (!skb)
+		goto failure;
 
 	rep = __nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
 			  NLMSG_ERROR, payload, flags);
 	errmsg = nlmsg_data(rep);
 	errmsg->error = err;
-	memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh));
+	unsafe_memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg)
+					 ?  nlh->nlmsg_len : sizeof(*nlh),
+		      /* "payload" was bounds checked against nlh->nlmsg_len,
+		       * and overflow-checked as tlvlen was constructed.
+		       */);
 
 	if (nlk_has_extack && extack) {
 		if (extack->_msg) {
@@ -2469,6 +2481,11 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 	nlmsg_end(skb, rep);
 
 	nlmsg_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid);
+	return;
+failure:
+	NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
+	sk_error_report(NETLINK_CB(in_skb).sk);
+
 }
 EXPORT_SYMBOL(netlink_ack);
 
-- 
2.34.1

Powered by blists - more mailing lists