| Message ID | 22ba689983844705563c15c2f16e6381f2a0412c.1682807958.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tipc: fix the mtu update in link mtu negotiation | expand |
>-----Original Message----- >From: Xin Long <lucien.xin@gmail.com> >Sent: Sunday, April 30, 2023 5:41 AM >To: network dev <netdev@vger.kernel.org>; tipc-discussion@lists.sourceforge.net >Cc: davem@davemloft.net; kuba@kernel.org; Eric Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; Jon Maloy ><jmaloy@redhat.com> >Subject: [PATCH net 2/2] tipc: do not update mtu if msg_max is too small > >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. > >Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation") >Reported-by: Shuang Li <shuali@redhat.com> >Signed-off-by: Xin Long <lucien.xin@gmail.com> >--- > net/tipc/link.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > >diff --git a/net/tipc/link.c b/net/tipc/link.c >index b3ce24823f50..a9e46c58b28a 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); >@@ -2283,8 +2283,9 @@ 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); >+ msg_max = msg_max_pkt(hdr); >+ if (msg_max >= tipc_bearer_min_mtu(l->net, l->bearer_id) && l->mtu > msg_max) >+ l->mtu = msg_max; If this link receives a malicious ACTIVATE_MSG from a peer, this message should be dropped. It is better if the check " msg_max < tipc_bearer_min_mtu()" is put at the beginning of this ACTIVATE_MSG handling and we break immediately. > break; > > case STATE_MSG: >-- >2.39.1
diff --git a/net/tipc/link.c b/net/tipc/link.c index b3ce24823f50..a9e46c58b28a 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); @@ -2283,8 +2283,9 @@ 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); + msg_max = msg_max_pkt(hdr); + if (msg_max >= tipc_bearer_min_mtu(l->net, l->bearer_id) && l->mtu > msg_max) + l->mtu = msg_max; break; case STATE_MSG:
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. Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation") Reported-by: Shuang Li <shuali@redhat.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/tipc/link.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)