lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <d7ccfcc9-b446-66ad-ab04-baa1cdbbe0ce@redhat.com> Date: Wed, 3 May 2023 15:29:07 -0400 From: Jon Maloy <jmaloy@...hat.com> To: Xin Long <lucien.xin@...il.com>, Tung Quang Nguyen <tung.q.nguyen@...tech.com.au> Cc: network dev <netdev@...r.kernel.org>, "tipc-discussion@...ts.sourceforge.net" <tipc-discussion@...ts.sourceforge.net>, "davem@...emloft.net" <davem@...emloft.net>, "kuba@...nel.org" <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com> Subject: Re: [PATCHv2 net 2/3] tipc: do not update mtu if msg_max is too small in mtu negotiation On 2023-05-03 09:35, Xin Long wrote: > On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen > <tung.q.nguyen@...tech.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.davem@davemloft.net/ > > unless there are some new rules I missed. I have not seen this one before, and I disagree with David here. Many of the changes between versions are trivial, and some comments even incomprehensible once the patch has been applied. I have always put them after the "---" comment, and I will continue to do so until David starts rejecting such patches. But ok, do as you find right. ///jon > > Thanks. > >>> Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation") >>> Reported-by: Shuang Li <shuali@...hat.com> >>> Signed-off-by: Xin Long <lucien.xin@...il.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
Powered by blists - more mailing lists