| Message ID | 20220901071336.1418572-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] netlink: Bounds-check struct nlmsgerr creation | expand |
On Thu, 1 Sep 2022 00:13:36 -0700 Kees Cook wrote: > rep = __nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq, > NLMSG_ERROR, payload, flags); All we should need here is __nlmsg_put() -> nlmsg_put(), that's idiomatic for netlink. > 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. > + */);
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));
On Fri, 2 Sep 2022 16:08:01 -0700 Kees Cook wrote: > > - 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.) The netlink helpers for adding attributes check whether there is enough space left in the skb. So if the calculation overflows, so be it. We'll hit EMSGSIZE in the writing phase and unwind. The writing should make no assumptions about the skb size. In fact all dumps will routinely hit EMSGSIZE as we try to fit as many objects into a skb as possible, so we unwind the one that would go over. Unwinding is well exercised in a lot of netlink code (not here, MSG_DONE/MSG_ERROR ain't a dump). The pre-calculation is just an estimate, if the message size ends up being insane it really doesn't matter if the calculation is 0, INT_MAX or random(). User is not gonna get a response, anyway. ... unless someone uses the unsafe helpers like __nlmsg_put() rather than nlmsg_put(), hence my suggestion in the other email.
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 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);
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> --- v3: add back ip_set_core patch v2: https://lore.kernel.org/lkml/20220901064858.1417126-1-keescook@chromium.org v1: https://lore.kernel.org/lkml/20220901030610.1121299-3-keescook@chromium.org --- net/netfilter/ipset/ip_set_core.c | 10 ++-- net/netlink/af_netlink.c | 81 +++++++++++++++++++------------ 2 files changed, 58 insertions(+), 33 deletions(-)