LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] tipc: fix a potential missing-check bug
@ 2018-05-01  4:26 Wenwen Wang
  2018-05-01 12:01 ` Jon Maloy
  0 siblings, 1 reply; 2+ messages in thread
From: Wenwen Wang @ 2018-05-01  4:26 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Jon Maloy, Ying Xue, David S. Miller,
	open list:TIPC NETWORK LAYER, open list:TIPC NETWORK LAYER,
	open list

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. That means the security check can potentially be
bypassed, especially when an adversary can control the length of "list".

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@umn.edu>
---
 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

^ permalink raw reply	[flat|nested] 2+ messages in thread

* RE: [PATCH] tipc: fix a potential missing-check bug
  2018-05-01  4:26 [PATCH] tipc: fix a potential missing-check bug Wenwen Wang
@ 2018-05-01 12:01 ` Jon Maloy
  0 siblings, 0 replies; 2+ messages in thread
From: Jon Maloy @ 2018-05-01 12:01 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Ying Xue, David S. Miller,
	open list:TIPC NETWORK LAYER, open list:TIPC NETWORK LAYER,
	open list



> -----Original Message-----
> From: Wenwen Wang [mailto:wang6495@umn.edu]
> Sent: Tuesday, May 01, 2018 00:26
> To: Wenwen Wang <wang6495@umn.edu>
> Cc: Kangjie Lu <kjlu@umn.edu>; Jon Maloy <jon.maloy@ericsson.com>; Ying
> Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>;
> open list:TIPC NETWORK LAYER <netdev@vger.kernel.org>; open list:TIPC
> NETWORK LAYER <tipc-discussion@lists.sourceforge.net>; open list <linux-
> kernel@vger.kernel.org>
> 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@umn.edu>
> ---
>  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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-05-01 12:01 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-01  4:26 [PATCH] tipc: fix a potential missing-check bug Wenwen Wang
2018-05-01 12:01 ` Jon Maloy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).