On Tue, May 03, 2022 at 10:31:05PM -0500, Gustavo A. R. Silva wrote: > On Tue, May 03, 2022 at 06:44:10PM -0700, Kees Cook wrote: > [...] > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > index 1b5a9c2e1c29..09346aee1022 100644 > > --- a/net/netlink/af_netlink.c > > +++ b/net/netlink/af_netlink.c > > @@ -2445,7 +2445,10 @@ void netlink_ack(struct sk_buff *in_skb, struct > > nlmsghdr *nlh, int err, > > NLMSG_ERROR, payload, flags); > > errmsg = nlmsg_data(rep); > > errmsg->error = err; > > - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : > > sizeof(*nlh)); > > + errmsg->msg = *nlh; > > + if (payload > sizeof(*errmsg)) > > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, > > + nlh->nlmsg_len - sizeof(*nlh)); > > They have nlmsg_len()[1] for the length of the payload without the header: > > /** > * nlmsg_len - length of message payload > * @nlh: netlink message header > */ > static inline int nlmsg_len(const struct nlmsghdr *nlh) > { > return nlh->nlmsg_len - NLMSG_HDRLEN; > }
Oh, hm, yeah, that would be much cleaner. The relationship between "payload" and nlmsg_len is confusing in here. :) So, this should be simpler: - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh)); + errmsg->msg = *nlh; + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, nlmsg_len(nlh)); It's actually this case that triggered my investigation in __bos(1)'s misbehavior around sub-structs, since this case wasn't getting silenced: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832 It still feels like it should be possible to get this right without splitting the memcpy, though. Hmpf. > (would that function use some sanitization, though? what if nlmsg_len is > somehow manipulated to be less than NLMSG_HDRLEN?...) Maybe something like: static inline int nlmsg_len(const struct nlmsghdr *nlh) { if (WARN_ON(nlh->nlmsg_len < NLMSG_HDRLEN)) return 0; return nlh->nlmsg_len - NLMSG_HDRLEN; } > Also, it seems there is at least one more instance of this same issue: > > diff --git a/net/netfilter/ipset/ip_set_core.c > b/net/netfilter/ipset/ip_set_core.c > index 16ae92054baa..d06184b94af5 100644 > --- a/net/netfilter/ipset/ip_set_core.c > +++ b/net/netfilter/ipset/ip_set_core.c > @@ -1723,7 +1723,8 @@ 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); > + errmsg->msg = *nlh; > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, > nlmsg_len(nlh)); Ah, yes, nice catch! > cmdattr = (void *)&errmsg->msg + min_len; > > ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, cmdattr, > > -- > Gustavo > > [1] > https://elixir.bootlin.com/linux/v5.18-rc5/source/include/net/netlink.h#L577 -- Kees Cook