[PATCH 0/2] netlink: Bounds-check struct nlmsgerr creation

[PATCH 0/2] netlink: Bounds-check struct nlmsgerr creation

[Kees Cook]

Hi,

In order to avoid triggering the coming runtime memcpy() bounds checking,
the length of the destination needs to be "visible" to the compiler in
some way. However, netlink is constructed in a rather hidden fashion,
and my attempts to wrangle it have resulted in this series, which perform
explicit bounds checking before using unsafe_memcpy().

-Kees

Kees Cook (2):
  netlink: Bounds-check nlmsg_len()
  netlink: Bounds-check struct nlmsgerr creation

 include/net/netlink.h             | 10 ++++++-
 net/netfilter/ipset/ip_set_core.c | 10 +++++--
 net/netlink/af_netlink.c          | 49 +++++++++++++++++++++----------
 3 files changed, 49 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/2] netlink: Bounds-check nlmsg_len()

[Kees Cook]

The nlmsg_len() helper returned "int" from a u32 calculation that could
possible go negative. WARN() if this calculation ever goes negative
(instead returning 0), or if the result would be larger than INT_MAX
(instead returning INT_MAX).

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Cc: Florian Westphal <fw@strlen.de>
Cc: syzbot <syzkaller@googlegroups.com>
Cc: Yajun Deng <yajun.deng@linux.dev>
Cc: netdev@vger.kernel.org
Cc: netfilter-devel@vger.kernel.org
Cc: coreteam@netfilter.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/net/netlink.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 7a2a9d3144ba..f8cb0543635e 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -576,7 +576,15 @@ static inline void *nlmsg_data(const struct nlmsghdr *nlh)
  */
 static inline int nlmsg_len(const struct nlmsghdr *nlh)
 {
-	return nlh->nlmsg_len - NLMSG_HDRLEN;
+	u32 nlmsg_contents_len;
+
+	if (WARN_ON_ONCE(check_sub_overflow(nlh->nlmsg_len,
+					    (u32)NLMSG_HDRLEN,
+					    &nlmsg_contents_len)))
+		return 0;
+	if (WARN_ON_ONCE(nlmsg_contents_len > INT_MAX))
+		return INT_MAX;
+	return nlmsg_contents_len;
 }
 
 /**
-- 
2.34.1

Re: [PATCH 1/2] netlink: Bounds-check nlmsg_len()

[Jakub Kicinski]

On Wed, 31 Aug 2022 20:06:09 -0700 Kees Cook wrote:
>  static inline int nlmsg_len(const struct nlmsghdr *nlh)
>  {
> -	return nlh->nlmsg_len - NLMSG_HDRLEN;
> +	u32 nlmsg_contents_len;
> +
> +	if (WARN_ON_ONCE(check_sub_overflow(nlh->nlmsg_len,
> +					    (u32)NLMSG_HDRLEN,
> +					    &nlmsg_contents_len)))
> +		return 0;
> +	if (WARN_ON_ONCE(nlmsg_contents_len > INT_MAX))
> +		return INT_MAX;
> +	return nlmsg_contents_len;

We check the messages on input, making sure the length is valid wrt
skb->len, and sane, ie > NLMSG_HDRLEN. See netlink_rcv_skb().

Can we not, pretty please? :(

Re: [PATCH 1/2] netlink: Bounds-check nlmsg_len()

[Kees Cook]

On Wed, Aug 31, 2022 at 08:18:25PM -0700, Jakub Kicinski wrote:
> On Wed, 31 Aug 2022 20:06:09 -0700 Kees Cook wrote:
> >  static inline int nlmsg_len(const struct nlmsghdr *nlh)
> >  {
> > -	return nlh->nlmsg_len - NLMSG_HDRLEN;
> > +	u32 nlmsg_contents_len;
> > +
> > +	if (WARN_ON_ONCE(check_sub_overflow(nlh->nlmsg_len,
> > +					    (u32)NLMSG_HDRLEN,
> > +					    &nlmsg_contents_len)))
> > +		return 0;
> > +	if (WARN_ON_ONCE(nlmsg_contents_len > INT_MAX))
> > +		return INT_MAX;
> > +	return nlmsg_contents_len;
> 
> We check the messages on input, making sure the length is valid wrt
> skb->len, and sane, ie > NLMSG_HDRLEN. See netlink_rcv_skb().
> 
> Can we not, pretty please? :(

This would catch corrupted values...

Is the concern the growth in image size? The check_sub_overflow() isn't
large at all -- it's just adding a single overflow bit test. The WARNs
are heavier, but they're all out-of-line.

-- 
Kees Cook

Re: [PATCH 1/2] netlink: Bounds-check nlmsg_len()

[Jakub Kicinski]

On Wed, 31 Aug 2022 23:27:08 -0700 Kees Cook wrote:
> This would catch corrupted values...
> 
> Is the concern the growth in image size? The check_sub_overflow() isn't
> large at all -- it's just adding a single overflow bit test. The WARNs
> are heavier, but they're all out-of-line.

It turns the most obvious function into a noodle bar :(

Looking at this function in particular is quite useful, because 
it clearly indicates that the nlmsg_len includes the header.

How about we throw in a

	WARN_ON_ONCE(nlh->nlmsg_len < NLMSG_HDRLEN ||
		     nlh->nlmsg_len > INT_MAX);

but leave the actual calculation human readable C?

Re: [PATCH 1/2] netlink: Bounds-check nlmsg_len()

[Eric Dumazet]

On Thu, Sep 1, 2022 at 12:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 31 Aug 2022 23:27:08 -0700 Kees Cook wrote:
> > This would catch corrupted values...
> >
> > Is the concern the growth in image size? The check_sub_overflow() isn't
> > large at all -- it's just adding a single overflow bit test. The WARNs
> > are heavier, but they're all out-of-line.
>
> It turns the most obvious function into a noodle bar :(
>
> Looking at this function in particular is quite useful, because
> it clearly indicates that the nlmsg_len includes the header.
>
> How about we throw in a
>
>         WARN_ON_ONCE(nlh->nlmsg_len < NLMSG_HDRLEN ||
>                      nlh->nlmsg_len > INT_MAX);
>
> but leave the actual calculation human readable C?

This is inlined, and will add a lot of extra code. We are making the
kernel slower at each release.

What about letting fuzzers like syzbot find the potential issues ?

DEBUG_NET_WARN_ON_ONCE(...);

[PATCH 2/2] 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.

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: Jakub Kicinski <kuba@kernel.org>
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>
---
 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

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

[Jakub Kicinski]

On Wed, 31 Aug 2022 20:06:10 -0700 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.

This one you gotta rebase we just rewrote the af_netlink 
part last week :)