On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen <tung.q.ngu...@dektech.com.au> wrote: > > >When doing link mtu negotiation, a malicious peer may send Activate msg > >with a very small mtu, e.g. 4 in Shuang's testing, without checking for > >the minimum mtu, l->mtu will be set to 4 in tipc_link_proto_rcv(), then > >n->links[bearer_id].mtu is set to 4294967228, which is a overflow of > >'4 - INT_H_SIZE - EMSG_OVERHEAD' in tipc_link_mss(). > > > >With tipc_link.mtu = 4, tipc_link_xmit() kept printing the warning: > > > > tipc: Too large msg, purging xmit list 1 5 0 40 4! > > tipc: Too large msg, purging xmit list 1 15 0 60 4! > > > >And with tipc_link_entry.mtu 4294967228, a huge skb was allocated in > >named_distribute(), and when purging it in tipc_link_xmit(), a crash > >was even caused: > > > > general protection fault, probably for non-canonical address > > 0x2100001011000dd: 0000 [#1] PREEMPT SMP PTI > > CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Not tainted 6.3.0.neta #19 > > RIP: 0010:kfree_skb_list_reason+0x7e/0x1f0 > > Call Trace: > > <IRQ> > > skb_release_data+0xf9/0x1d0 > > kfree_skb_reason+0x40/0x100 > > tipc_link_xmit+0x57a/0x740 [tipc] > > tipc_node_xmit+0x16c/0x5c0 [tipc] > > tipc_named_node_up+0x27f/0x2c0 [tipc] > > tipc_node_write_unlock+0x149/0x170 [tipc] > > tipc_rcv+0x608/0x740 [tipc] > > tipc_udp_recv+0xdc/0x1f0 [tipc] > > udp_queue_rcv_one_skb+0x33e/0x620 > > udp_unicast_rcv_skb.isra.72+0x75/0x90 > > __udp4_lib_rcv+0x56d/0xc20 > > ip_protocol_deliver_rcu+0x100/0x2d0 > > > >This patch fixes it by checking the new mtu against tipc_bearer_min_mtu(), > >and not updating mtu if it is too small. > > > >v1->v2: > > - do the msg_max check against the min MTU early, as Tung suggested. > Please move above version change comment to after "---". I think it's correct to NOT use ''---' for version changes, see the comment from davem:
https://lore.kernel.org/netdev/20160415.172858.253625178036493951.da...@davemloft.net/ unless there are some new rules I missed. Thanks. > > > >Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation") > >Reported-by: Shuang Li <shu...@redhat.com> > >Signed-off-by: Xin Long <lucien....@gmail.com> > >--- > > net/tipc/link.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > >diff --git a/net/tipc/link.c b/net/tipc/link.c > >index b3ce24823f50..2eff1c7949cb 100644 > >--- a/net/tipc/link.c > >+++ b/net/tipc/link.c > >@@ -2200,7 +2200,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, > >struct sk_buff *skb, > > struct tipc_msg *hdr = buf_msg(skb); > > struct tipc_gap_ack_blks *ga = NULL; > > bool reply = msg_probe(hdr), retransmitted = false; > >- u32 dlen = msg_data_sz(hdr), glen = 0; > >+ u32 dlen = msg_data_sz(hdr), glen = 0, msg_max; > > u16 peers_snd_nxt = msg_next_sent(hdr); > > u16 peers_tol = msg_link_tolerance(hdr); > > u16 peers_prio = msg_linkprio(hdr); > >@@ -2239,6 +2239,9 @@ static int tipc_link_proto_rcv(struct tipc_link *l, > >struct sk_buff *skb, > > switch (mtyp) { > > case RESET_MSG: > > case ACTIVATE_MSG: > >+ msg_max = msg_max_pkt(hdr); > >+ if (msg_max < tipc_bearer_min_mtu(l->net, l->bearer_id)) > >+ break; > > /* Complete own link name with peer's interface name */ > > if_name = strrchr(l->name, ':') + 1; > > if (sizeof(l->name) - (if_name - l->name) <= TIPC_MAX_IF_NAME) > >@@ -2283,8 +2286,8 @@ static int tipc_link_proto_rcv(struct tipc_link *l, > >struct sk_buff *skb, > > l->peer_session = msg_session(hdr); > > l->in_session = true; > > l->peer_bearer_id = msg_bearer_id(hdr); > >- if (l->mtu > msg_max_pkt(hdr)) > >- l->mtu = msg_max_pkt(hdr); > >+ if (l->mtu > msg_max) > >+ l->mtu = msg_max; > > break; > > > > case STATE_MSG: > >-- > >2.39.1 > _______________________________________________ tipc-discussion mailing list tipc-discussion@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tipc-discussion
