RE: [PATCH] tipc: fix a potential missing-check bug

[Jon Maloy]

> -----Original Message-----
> From: Wenwen Wang [mailto:wang6495@xxxxxxx]
> Sent: Tuesday, May 01, 2018 00:26
> To: Wenwen Wang <wang6495@xxxxxxx>
> Cc: Kangjie Lu <kjlu@xxxxxxx>; Jon Maloy <jon.maloy@xxxxxxxxxxxx>; Ying
> Xue <ying.xue@xxxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>;
> open list:TIPC NETWORK LAYER <netdev@xxxxxxxxxxxxxxx>; open list:TIPC
> NETWORK LAYER <tipc-discussion@xxxxxxxxxxxxxxxxxxxxx>; open list <linux-
> kernel@xxxxxxxxxxxxxxx>
> Subject: [PATCH] tipc: fix a potential missing-check bug
> 
> In tipc_link_xmit(), the member field "len" of l->backlog[imp] must be less
> than the member field "limit" of l->backlog[imp] when imp is equal to
> TIPC_SYSTEM_IMPORTANCE. Otherwise, an error code, i.e., -ENOBUFS, is
> returned. This is enforced by the security check. However, at the end of
> tipc_link_xmit(), the length of "list" is added to l->backlog[imp].len without
> any further check. This can potentially cause unexpected values for
> l->backlog[imp].len. If imp is equal to TIPC_SYSTEM_IMPORTANCE and the
> original value of l->backlog[imp].len is less than l->backlog[imp].limit, after
> this addition, l->backlog[imp] could be larger than
> l->backlog[imp].limit. 

It can, but only once. That is the intention with allowing oversubscription. This is expected and permitted.
At next sending attempt, if the send queue has not been reduced in the meantime, the link will be reset, as intended.

> That means the security check can potentially be
> bypassed,  especially when an adversary can control the length of "list".

The length of 'list' is entirely controlled by TIPC itself, either by the socket layer (where length  always is 1 for this type of messages) or
 name_dist, In the latter case the length is also 1, except at first link setup, when there guaranteed is no congestion anyway.

I appreciate your interest, but this patch is not needed.

BR
///jon

> 
> This patch performs such a check after the modification to
> l->backlog[imp].len (if imp is TIPC_SYSTEM_IMPORTANCE) to avoid such
> security issues. An error code will be returned if an unexpected value of
> l->backlog[imp].len is generated.
> 
> Signed-off-by: Wenwen Wang <wang6495@xxxxxxx>
> ---
>  net/tipc/link.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c index 695acb7..62972fa 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -948,6 +948,11 @@ int tipc_link_xmit(struct tipc_link *l, struct
> sk_buff_head *list,
>  			continue;
>  		}
>  		l->backlog[imp].len += skb_queue_len(list);
> +		if (imp == TIPC_SYSTEM_IMPORTANCE &&
> +		    l->backlog[imp].len >= l->backlog[imp].limit) {
> +			pr_warn("%s<%s>, link overflow", link_rst_msg, l-
> >name);
> +			return -ENOBUFS;
> +		}
>  		skb_queue_splice_tail_init(list, backlogq);
>  	}
>  	l->snd_nxt = seqno;
> --
> 2.7.4