> -----Original Message-----
> From: Xin Long <[email protected]>
> Sent: 21-Dec-18 05:29
> To: Jon Maloy <[email protected]>
> Cc: Ying Xue <[email protected]>; [email protected]; tipc-
> [email protected]
> Subject: Re: [tipc-discussion] FW: [net 1/1] tipc: sanity check on received
> netlink buffer
>
> 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.
>
According to Yings's previous comment that doesn't seem to be necessary.
///jon
> >
> > 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
