On Fri, Dec 21, 2018 at 12:35 AM Jon Maloy <[email protected]> wrote: > > Hi Ying and Xin, > Any viewpoints on this before I send it in? > It should be noted that skb->tail in the worst case will be the same as > skb->end, which points to the first byte of the skb_shared_info area. > Lucklily, and not only due to luck, I think, this byte happens to be named " > __unused" in that structure. So the change should be safe, and my tests have > not revealed any problems. Whether this really solves the problem reported by > syzbot I don't know, since I am unable to reproduce it, but this is my take > on it.
I built a KMSAN env a couple of weeks ago, but it's gone now. I can rebuild one and try to reproduce it if you still need. > > Regards > ///jon > > > -----Original Message----- > From: Jon Maloy > Sent: 18-Dec-18 15:29 > To: Jon Maloy <[email protected]>; Jon Maloy <[email protected]> > Cc: Mohan Krishna Ghanta Krishnamurthy > <[email protected]>; > [email protected]; Tung Quang Nguyen > <[email protected]>; Hoang Huu Le <[email protected]>; > Canh Duc Luu <[email protected]>; Tuong Tong Lien > <[email protected]>; Gordan Mihaljevic > <[email protected]>; [email protected]; > [email protected] > Subject: [net 1/1] tipc: sanity check on received netlink buffer > > When tipc receives a sk buffer in tipc_net_link_compat_rcv() it performs no > controls that the buffer has the required minimum size. Furthermore, the > buffer may contain a string, which we have no guarantee is zero- terminated. > > We now introduce a check that the buffer at least is large enough to contain > a generic and a TIPC specific netlink header, since those must be present in > all valid messages. > > We also set the buffer tail to point to a zero character. This ensures that > subsequent string operations on buffer data never can fail, even if the given > string is invalid. > > Reported-by: [email protected] > Reported-by: [email protected] > Signed-off-by: Jon Maloy <[email protected]> > --- > net/tipc/netlink_compat.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c index > 6376467..b37ed6e 100644 > --- a/net/tipc/netlink_compat.c > +++ b/net/tipc/netlink_compat.c > @@ -1188,6 +1188,13 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, > struct genl_info *info) > > memset(&msg, 0, sizeof(msg)); > > + if (skb_headlen(skb) < GENL_HDRLEN + TIPC_GENL_HDRLEN) { > + err = -EINVAL; > + msg.rep = tipc_get_err_tlv(TIPC_CFG_TLV_ERROR); > + goto send; > + } 1. This check may not be necessary, as genl_family_rcv_msg() could cover it, no? hdrlen = GENL_HDRLEN + family->hdrsize; if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) return -EINVAL; > + *skb_tail_pointer(skb) = 0; > + It may be a little bit tricky, but yes, a very easy fix. I'm thinking: 2. https://www.spinics.net/lists/netdev/msg540733.html This is a common issue in TIPC netlink when parsing name string. I was looking at the processing for ifname in rtnl_setlink(), and it's using nla_strlcpy(). So maybe it's better to do the same here? --- a/net/tipc/bearer.c +++ b/net/tipc/bearer.c @@ -857,7 +857,7 @@ int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info) int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info) { int err; - char *bearer; + char bearer[TIPC_MAX_BEARER_NAME]; struct nlattr *attrs[TIPC_NLA_BEARER_MAX + 1]; struct net *net = sock_net(skb->sk); u32 domain = 0; @@ -868,6 +868,7 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info) if (!info->attrs[TIPC_NLA_BEARER]) return -EINVAL; + nla_strlcpy(bearer, info->attrs[TIPC_NLA_BEARER], TIPC_MAX_BEARER_NAME); err = nla_parse_nested(attrs, TIPC_NLA_BEARER_MAX, info->attrs[TIPC_NLA_BEARER], tipc_nl_bearer_policy, info->extack); 3. https://www.spinics.net/lists/netdev/msg540734.html the similar thing below in tipc_nl_compat_link_set()? but should do something more for tipc_link_config which is more than a string. @@ -723,19 +723,21 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd, struct sk_buff *skb, struct tipc_nl_compat_msg *msg) { - struct tipc_link_config *lc; + int len = TLV_GET_LEN(msg->req) - TLV_LENGTH(0); + struct tipc_link_config lc = {0}; struct tipc_bearer *bearer; struct tipc_media *media; - lc = (struct tipc_link_config *)TLV_DATA(msg->req); + memcpy(&lc, TLV_DATA(msg->req), + len >= sizeof(lc) ? sizeof(lc) - 1 : len); - media = tipc_media_find(lc->name); + media = tipc_media_find(lc.name); > req_nlh = (struct nlmsghdr *)skb->data; > msg.req = nlmsg_data(req_nlh) + GENL_HDRLEN + TIPC_GENL_HDRLEN; > msg.cmd = req_userhdr->cmd; > -- > 2.1.4 > > > > _______________________________________________ > tipc-discussion mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/tipc-discussion _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
