[PATCH v2] netlink: Bounds-check struct nlmsgerr creation

[PATCH v2] netlink: Bounds-check struct nlmsgerr creation

[Kees Cook]

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. Avoid this future run-time warning:

  memcpy: detected field-spanning write (size 32) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16)

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Cc: Florian Westphal <fw@strlen.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: syzbot <syzkaller@googlegroups.com>
Cc: netfilter-devel@vger.kernel.org
Cc: coreteam@netfilter.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2: Rebased to -next
v1: https://lore.kernel.org/lkml/20220901030610.1121299-3-keescook@chromium.org
---
 net/netlink/af_netlink.c | 81 +++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 30 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f89ba302ac6e..1285779d9ab6 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2400,35 +2400,44 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(__netlink_dump_start);
 
-static size_t
+/* Returns false on overflow */
+static bool __must_check
 netlink_ack_tlv_len(struct netlink_sock *nlk, int err,
-		    const struct netlink_ext_ack *extack)
+		    const struct netlink_ext_ack *extack,
+		    size_t *tlvlen)
 {
-	size_t tlvlen;
+	*tlvlen = 0;
 
 	if (!extack || !(nlk->flags & NETLINK_F_EXT_ACK))
-		return 0;
+		return true;
 
-	tlvlen = 0;
-	if (extack->_msg)
-		tlvlen += nla_total_size(strlen(extack->_msg) + 1);
-	if (extack->cookie_len)
-		tlvlen += nla_total_size(extack->cookie_len);
+	if (extack->_msg &&
+	    check_add_overflow(*tlvlen, nla_total_size(strlen(extack->_msg) + 1), tlvlen))
+		return false;
+
+	if (extack->cookie_len &&
+	    check_add_overflow(*tlvlen, nla_total_size(extack->cookie_len), tlvlen))
+		return false;
 
 	/* Following attributes are only reported as error (not warning) */
 	if (!err)
-		return tlvlen;
+		return true;
 
-	if (extack->bad_attr)
-		tlvlen += nla_total_size(sizeof(u32));
-	if (extack->policy)
-		tlvlen += netlink_policy_dump_attr_size_estimate(extack->policy);
-	if (extack->miss_type)
-		tlvlen += nla_total_size(sizeof(u32));
-	if (extack->miss_nest)
-		tlvlen += nla_total_size(sizeof(u32));
+	if (extack->bad_attr &&
+	    check_add_overflow(*tlvlen, nla_total_size(sizeof(u32)), tlvlen))
+		return false;
+	if (extack->policy &&
+	    check_add_overflow(*tlvlen, netlink_policy_dump_attr_size_estimate(extack->policy),
+			       tlvlen))
+		return false;
+	if (extack->miss_type &&
+	    check_add_overflow(*tlvlen, nla_total_size(sizeof(u32)), tlvlen))
+		return false;
+	if (extack->miss_nest &&
+	    check_add_overflow(*tlvlen, nla_total_size(sizeof(u32)), tlvlen))
+		return false;
 
-	return tlvlen;
+	return true;
 }
 
 static void
@@ -2472,33 +2481,39 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 	size_t payload = sizeof(*errmsg);
 	struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
 	unsigned int flags = 0;
-	size_t tlvlen;
+	size_t alloc_size, tlvlen = 0;
 
 	/* Error messages get the original request appened, unless the user
 	 * requests to cap the error message, and get extra error data if
 	 * requested.
 	 */
-	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;
 
-	tlvlen = netlink_ack_tlv_len(nlk, err, extack);
+	if (!netlink_ack_tlv_len(nlk, err, extack, &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 (tlvlen)
 		netlink_ack_tlv_fill(in_skb, skb, nlh, err, extack);
@@ -2506,6 +2521,12 @@ 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);
+	return;
 }
 EXPORT_SYMBOL(netlink_ack);
 
-- 
2.34.1

Re: [PATCH v2] netlink: Bounds-check struct nlmsgerr creation

[Jozsef Kadlecsik]

Hi,

On Wed, 31 Aug 2022, Kees Cook wrote:

> 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. Avoid this future run-time warning:
> 
>   memcpy: detected field-spanning write (size 32) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16)
> 
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: syzbot <syzkaller@googlegroups.com>
> Cc: netfilter-devel@vger.kernel.org
> Cc: coreteam@netfilter.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v2: Rebased to -next
> v1: https://lore.kernel.org/lkml/20220901030610.1121299-3-keescook@chromium.org
> ---
>  net/netlink/af_netlink.c | 81 +++++++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 30 deletions(-)

Could you add back the net/netfilter/ipset/ip_set_core.c part? Thanks!

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2] netlink: Bounds-check struct nlmsgerr creation

[Kees Cook]

On Thu, Sep 01, 2022 at 09:06:03AM +0200, Jozsef Kadlecsik wrote:
> Hi,
> 
> On Wed, 31 Aug 2022, Kees Cook wrote:
> 
> > 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. Avoid this future run-time warning:
> > 
> >   memcpy: detected field-spanning write (size 32) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16)
> > 
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> > Cc: Florian Westphal <fw@strlen.de>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: syzbot <syzkaller@googlegroups.com>
> > Cc: netfilter-devel@vger.kernel.org
> > Cc: coreteam@netfilter.org
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > v2: Rebased to -next
> > v1: https://lore.kernel.org/lkml/20220901030610.1121299-3-keescook@chromium.org
> > ---
> >  net/netlink/af_netlink.c | 81 +++++++++++++++++++++++++---------------
> >  1 file changed, 51 insertions(+), 30 deletions(-)
> 
> Could you add back the net/netfilter/ipset/ip_set_core.c part? Thanks!

*face palm* Yes, thank you. v3 on the way.

-- 
Kees Cook