Re: [tipc-discussion] [PATCH net 2/2] tipc: do not update mtu if msg_max is too small

[Xin L. <luc...@gm...>]

On Mon, May 1, 2023 at 1:22 AM Tung Quang Nguyen <
tun...@de...> wrote:

>
>
> >-----Original Message-----
> >From: Xin Long <luc...@gm...>
> >Sent: Sunday, April 30, 2023 5:41 AM
> >To: network dev <ne...@vg...>;
> tip...@li...
> >Cc: da...@da...; ku...@ke...; Eric Dumazet <
> edu...@go...>; Paolo Abeni <pa...@re...>; Jon Maloy
> ><jm...@re...>
> >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 <sh...@re...>
> >Signed-off-by: Xin Long <luc...@gm...>
> >---
> > 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.
>
> Sounds good, will move 'msg_max < tipc_bearer_min_mtu()' up.

Thanks for reviewing.