| Message ID | 20220901030610.1121299-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netlink: Bounds-check struct nlmsgerr creation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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? :(
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.
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?
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(...);
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; } /**
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(-)