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

[Kees Cook <keescook@chromium.org>]

On Thu, Sep 01, 2022 at 12:13:36AM -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. 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)

To get rid of the above warning...

> [...]
> -		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.
> +			       */);

above is the "fix", since the compiler has no way to know how to bounds
check the arguments. But, to write that comment, I added all these
things:

> [...]
> -	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 that's not desirable, then I guess the question I want to ask is
"what can I put in the unsafe_memcpy() comment above that proves these
values have been sanity checked? In other words, how do we know that
tlvlen hasn't overflowed? (I don't know what other sanity checking may
have already happened, so I'm looking directly at the size calculations
here.)

I assume this isn't more desirable:

-	if (extack->cookie_len)
-		tlvlen += nla_total_size(extack->cookie_len);
+	if (extack->cookie_len) {
+		size_t len = nla_total_size(extack->cookie_len);
+
+		if (WARN_ON_ONCE(len > SIZE_MAX - tlvlen))
+			return 0;
+		tlvlen += len;
+	}

Or maybe wrap it nicely with a local macro and return 0 instead of
trying to pass an error up a layer?

+#define TLVADD(amount)	do { \
+	if (WARN_ON_ONCE(check_add_overflow(tlvlen, amount, &tlvlen))) \
+		return 0; \
+} while (0)

...
	if (extack->cookie_len)
-		tlvlen += nla_total_size(extack->cookie_len);
+		TLVADD(nla_total_size(extack->cookie_len));

-- 
Kees Cook