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

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: <202209021555.9EE2FBD3A@keescook>
Date:   Fri, 2 Sep 2022 16:08:01 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     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,
        Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3] netlink: Bounds-check struct nlmsgerr creation

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