From: Xin L. <luc...@gm...> - 2023-05-02 22:13:23
|
This patchset fixes a crash caused by a too small MTU carried in the activate msg. Note that as such malicious packet does not exist in the normal env, the fix won't break any application The 1st patch introduces a function to calculate the minimum MTU for the bearer, and the 2nd patch fixes the crash with this helper. While at it, the 3rd patch fixes the udp bearer mtu update by netlink with this helper. Xin Long (3): tipc: add tipc_bearer_min_mtu to calculate min mtu tipc: do not update mtu if msg_max is too small in mtu negotiation tipc: check the bearer min mtu properly when setting it by netlink net/tipc/bearer.c | 17 +++++++++++++++-- net/tipc/bearer.h | 3 +++ net/tipc/link.c | 9 ++++++--- net/tipc/udp_media.c | 5 +++-- 4 files changed, 27 insertions(+), 7 deletions(-) -- 2.39.1 |
From: Xin L. <luc...@gm...> - 2023-05-14 19:52:39
|
This patchset fixes a crash caused by a too small MTU carried in the activate msg. Note that as such malicious packet does not exist in the normal env, the fix won't break any application The 1st patch introduces a function to calculate the minimum MTU for the bearer, and the 2nd patch fixes the crash with this helper. While at it, the 3rd patch fixes the udp bearer mtu update by netlink with this helper. Xin Long (3): tipc: add tipc_bearer_min_mtu to calculate min mtu tipc: do not update mtu if msg_max is too small in mtu negotiation tipc: check the bearer min mtu properly when setting it by netlink net/tipc/bearer.c | 17 +++++++++++++++-- net/tipc/bearer.h | 3 +++ net/tipc/link.c | 9 ++++++--- net/tipc/udp_media.c | 5 +++-- 4 files changed, 27 insertions(+), 7 deletions(-) -- 2.39.1 |
From: Xin L. <luc...@gm...> - 2023-05-14 19:52:39
|
As different media may requires different min mtu, and even the same media with different net family requires different min mtu, add tipc_bearer_min_mtu() to calculate min mtu accordingly. This API will be used to check the new mtu when doing the link mtu negotiation in the next patch. Signed-off-by: Xin Long <luc...@gm...> Acked-by: Jon Maloy <jm...@re...> --- v2: - use bearer_get() to avoid the open code. v3: - move the change history under ---, as Tung suggested. --- net/tipc/bearer.c | 13 +++++++++++++ net/tipc/bearer.h | 3 +++ net/tipc/udp_media.c | 5 +++-- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index 35cac7733fd3..0e9a29e1536b 100644 --- a/net/tipc/bearer.c +++ b/net/tipc/bearer.c @@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id) return mtu; } +int tipc_bearer_min_mtu(struct net *net, u32 bearer_id) +{ + int mtu = TIPC_MIN_BEARER_MTU; + struct tipc_bearer *b; + + rcu_read_lock(); + b = bearer_get(net, bearer_id); + if (b) + mtu += b->encap_hlen; + rcu_read_unlock(); + return mtu; +} + /* tipc_bearer_xmit_skb - sends buffer to destination over bearer */ void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h index 490ad6e5f7a3..bd0cc5c287ef 100644 --- a/net/tipc/bearer.h +++ b/net/tipc/bearer.h @@ -146,6 +146,7 @@ struct tipc_media { * @identity: array index of this bearer within TIPC bearer array * @disc: ptr to link setup request * @net_plane: network plane ('A' through 'H') currently associated with bearer + * @encap_hlen: encap headers length * @up: bearer up flag (bit 0) * @refcnt: tipc_bearer reference counter * @@ -170,6 +171,7 @@ struct tipc_bearer { u32 identity; struct tipc_discoverer *disc; char net_plane; + u16 encap_hlen; unsigned long up; refcount_t refcnt; }; @@ -232,6 +234,7 @@ int tipc_bearer_setup(void); void tipc_bearer_cleanup(void); void tipc_bearer_stop(struct net *net); int tipc_bearer_mtu(struct net *net, u32 bearer_id); +int tipc_bearer_min_mtu(struct net *net, u32 bearer_id); bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id); void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, struct sk_buff *skb, diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index c2bb818704c8..0a85244fd618 100644 --- a/net/tipc/udp_media.c +++ b/net/tipc/udp_media.c @@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, udp_conf.local_ip.s_addr = local.ipv4.s_addr; udp_conf.use_udp_checksums = false; ub->ifindex = dev->ifindex; - if (tipc_mtu_bad(dev, sizeof(struct iphdr) + - sizeof(struct udphdr))) { + b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr); + if (tipc_mtu_bad(dev, b->encap_hlen)) { err = -EINVAL; goto err; } @@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, else udp_conf.local_ip6 = local.ipv6; ub->ifindex = dev->ifindex; + b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr); b->mtu = 1280; #endif } else { -- 2.39.1 |
From: Xin L. <luc...@gm...> - 2023-05-14 19:52:39
|
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...> Acked-by: Jon Maloy <jm...@re...> --- v2: - do the msg_max check against the min MTU early, as Tung suggested. v3: - move the change history under ---, as Tung suggested. --- 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 |
From: Xin L. <luc...@gm...> - 2023-05-14 19:52:40
|
Checking the bearer min mtu with tipc_udp_mtu_bad() only works for IPv4 UDP bearer, and IPv6 UDP bearer has a different value for the min mtu. This patch checks with encap_hlen + TIPC_MIN_BEARER_MTU for min mtu, which works for both IPv4 and IPv6 UDP bearer. Note that tipc_udp_mtu_bad() is still used to check media min mtu in __tipc_nl_media_set(), as m->mtu currently is only used by the IPv4 UDP bearer as its default mtu value. Fixes: 682cd3cf946b ("tipc: confgiure and apply UDP bearer MTU on running links") Signed-off-by: Xin Long <luc...@gm...> Acked-by: Jon Maloy <jm...@re...> --- net/tipc/bearer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index 0e9a29e1536b..53881406e200 100644 --- a/net/tipc/bearer.c +++ b/net/tipc/bearer.c @@ -1151,8 +1151,8 @@ int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info) return -EINVAL; } #ifdef CONFIG_TIPC_MEDIA_UDP - if (tipc_udp_mtu_bad(nla_get_u32 - (props[TIPC_NLA_PROP_MTU]))) { + if (nla_get_u32(props[TIPC_NLA_PROP_MTU]) < + b->encap_hlen + TIPC_MIN_BEARER_MTU) { NL_SET_ERR_MSG(info->extack, "MTU value is out-of-range"); return -EINVAL; -- 2.39.1 |
From: Tung Q. N. <tun...@de...> - 2023-05-15 02:41:39
|
>Subject: [PATCHv3 net 0/3] tipc: fix the mtu update in link mtu negotiation > >This patchset fixes a crash caused by a too small MTU carried in the >activate msg. Note that as such malicious packet does not exist in >the normal env, the fix won't break any application > >The 1st patch introduces a function to calculate the minimum MTU for >the bearer, and the 2nd patch fixes the crash with this helper. While >at it, the 3rd patch fixes the udp bearer mtu update by netlink with >this helper. > >Xin Long (3): > tipc: add tipc_bearer_min_mtu to calculate min mtu > tipc: do not update mtu if msg_max is too small in mtu negotiation > tipc: check the bearer min mtu properly when setting it by netlink > > net/tipc/bearer.c | 17 +++++++++++++++-- > net/tipc/bearer.h | 3 +++ > net/tipc/link.c | 9 ++++++--- > net/tipc/udp_media.c | 5 +++-- > 4 files changed, 27 insertions(+), 7 deletions(-) > >-- >2.39.1 For the whole series: Reviewed-by: Tung Nguyen <tun...@de...> |
From: Xin L. <luc...@gm...> - 2023-05-02 22:13:27
|
Checking the bearer min mtu with tipc_udp_mtu_bad() only works for IPv4 UDP bearer, and IPv6 UDP bearer has a different value for the min mtu. This patch checks with encap_hlen + TIPC_MIN_BEARER_MTU for min mtu, which works for both IPv4 and IPv6 UDP bearer. Note that tipc_udp_mtu_bad() is still used to check media min mtu in __tipc_nl_media_set(), as m->mtu currently is only used by the IPv4 UDP bearer as its default mtu value. Fixes: 682cd3cf946b ("tipc: confgiure and apply UDP bearer MTU on running links") Signed-off-by: Xin Long <luc...@gm...> --- net/tipc/bearer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index 0e9a29e1536b..53881406e200 100644 --- a/net/tipc/bearer.c +++ b/net/tipc/bearer.c @@ -1151,8 +1151,8 @@ int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info) return -EINVAL; } #ifdef CONFIG_TIPC_MEDIA_UDP - if (tipc_udp_mtu_bad(nla_get_u32 - (props[TIPC_NLA_PROP_MTU]))) { + if (nla_get_u32(props[TIPC_NLA_PROP_MTU]) < + b->encap_hlen + TIPC_MIN_BEARER_MTU) { NL_SET_ERR_MSG(info->extack, "MTU value is out-of-range"); return -EINVAL; -- 2.39.1 |
From: Xin L. <luc...@gm...> - 2023-05-02 22:13:28
|
As different media may requires different min mtu, and even the same media with different net family requires different min mtu, add tipc_bearer_min_mtu() to calculate min mtu accordingly. This API will be used to check the new mtu when doing the link mtu negotiation in the next patch. v1->v2: - use bearer_get() to avoid the open code. Signed-off-by: Xin Long <luc...@gm...> --- net/tipc/bearer.c | 13 +++++++++++++ net/tipc/bearer.h | 3 +++ net/tipc/udp_media.c | 5 +++-- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index 35cac7733fd3..0e9a29e1536b 100644 --- a/net/tipc/bearer.c +++ b/net/tipc/bearer.c @@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id) return mtu; } +int tipc_bearer_min_mtu(struct net *net, u32 bearer_id) +{ + int mtu = TIPC_MIN_BEARER_MTU; + struct tipc_bearer *b; + + rcu_read_lock(); + b = bearer_get(net, bearer_id); + if (b) + mtu += b->encap_hlen; + rcu_read_unlock(); + return mtu; +} + /* tipc_bearer_xmit_skb - sends buffer to destination over bearer */ void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h index 490ad6e5f7a3..bd0cc5c287ef 100644 --- a/net/tipc/bearer.h +++ b/net/tipc/bearer.h @@ -146,6 +146,7 @@ struct tipc_media { * @identity: array index of this bearer within TIPC bearer array * @disc: ptr to link setup request * @net_plane: network plane ('A' through 'H') currently associated with bearer + * @encap_hlen: encap headers length * @up: bearer up flag (bit 0) * @refcnt: tipc_bearer reference counter * @@ -170,6 +171,7 @@ struct tipc_bearer { u32 identity; struct tipc_discoverer *disc; char net_plane; + u16 encap_hlen; unsigned long up; refcount_t refcnt; }; @@ -232,6 +234,7 @@ int tipc_bearer_setup(void); void tipc_bearer_cleanup(void); void tipc_bearer_stop(struct net *net); int tipc_bearer_mtu(struct net *net, u32 bearer_id); +int tipc_bearer_min_mtu(struct net *net, u32 bearer_id); bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id); void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, struct sk_buff *skb, diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index c2bb818704c8..0a85244fd618 100644 --- a/net/tipc/udp_media.c +++ b/net/tipc/udp_media.c @@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, udp_conf.local_ip.s_addr = local.ipv4.s_addr; udp_conf.use_udp_checksums = false; ub->ifindex = dev->ifindex; - if (tipc_mtu_bad(dev, sizeof(struct iphdr) + - sizeof(struct udphdr))) { + b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr); + if (tipc_mtu_bad(dev, b->encap_hlen)) { err = -EINVAL; goto err; } @@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, else udp_conf.local_ip6 = local.ipv6; ub->ifindex = dev->ifindex; + b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr); b->mtu = 1280; #endif } else { -- 2.39.1 |
From: Xin L. <luc...@gm...> - 2023-05-02 22:13:27
|
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. Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation") Reported-by: Shuang Li <sh...@re...> Signed-off-by: Xin Long <luc...@gm...> --- 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 |
From: Tung Q. N. <tun...@de...> - 2023-05-03 03:47:07
|
>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 "---". > >Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation") >Reported-by: Shuang Li <sh...@re...> >Signed-off-by: Xin Long <luc...@gm...> >--- > 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 |
From: Tung Q. N. <tun...@de...> - 2023-05-03 03:52:05
|
>As different media may requires different min mtu, and even the >same media with different net family requires different min mtu, >add tipc_bearer_min_mtu() to calculate min mtu accordingly. > >This API will be used to check the new mtu when doing the link >mtu negotiation in the next patch. > >v1->v2: > - use bearer_get() to avoid the open code. This version change comment does not seem right. Please correct it to avoid confusion and put it after "---". > >Signed-off-by: Xin Long <luc...@gm...> >--- > net/tipc/bearer.c | 13 +++++++++++++ > net/tipc/bearer.h | 3 +++ > net/tipc/udp_media.c | 5 +++-- > 3 files changed, 19 insertions(+), 2 deletions(-) > >diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c >index 35cac7733fd3..0e9a29e1536b 100644 >--- a/net/tipc/bearer.c >+++ b/net/tipc/bearer.c >@@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id) > return mtu; > } > >+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id) >+{ >+ int mtu = TIPC_MIN_BEARER_MTU; >+ struct tipc_bearer *b; >+ >+ rcu_read_lock(); >+ b = bearer_get(net, bearer_id); >+ if (b) >+ mtu += b->encap_hlen; >+ rcu_read_unlock(); >+ return mtu; >+} >+ > /* tipc_bearer_xmit_skb - sends buffer to destination over bearer > */ > void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, >diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h >index 490ad6e5f7a3..bd0cc5c287ef 100644 >--- a/net/tipc/bearer.h >+++ b/net/tipc/bearer.h >@@ -146,6 +146,7 @@ struct tipc_media { > * @identity: array index of this bearer within TIPC bearer array > * @disc: ptr to link setup request > * @net_plane: network plane ('A' through 'H') currently associated with bearer >+ * @encap_hlen: encap headers length > * @up: bearer up flag (bit 0) > * @refcnt: tipc_bearer reference counter > * >@@ -170,6 +171,7 @@ struct tipc_bearer { > u32 identity; > struct tipc_discoverer *disc; > char net_plane; >+ u16 encap_hlen; > unsigned long up; > refcount_t refcnt; > }; >@@ -232,6 +234,7 @@ int tipc_bearer_setup(void); > void tipc_bearer_cleanup(void); > void tipc_bearer_stop(struct net *net); > int tipc_bearer_mtu(struct net *net, u32 bearer_id); >+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id); > bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id); > void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, > struct sk_buff *skb, >diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c >index c2bb818704c8..0a85244fd618 100644 >--- a/net/tipc/udp_media.c >+++ b/net/tipc/udp_media.c >@@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, > udp_conf.local_ip.s_addr = local.ipv4.s_addr; > udp_conf.use_udp_checksums = false; > ub->ifindex = dev->ifindex; >- if (tipc_mtu_bad(dev, sizeof(struct iphdr) + >- sizeof(struct udphdr))) { >+ b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr); >+ if (tipc_mtu_bad(dev, b->encap_hlen)) { I agree that calling tipc_mtu_bad() is not necessary for UDP bearer enabling. You can remove it in this patch. > err = -EINVAL; > goto err; > } >@@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, > else > udp_conf.local_ip6 = local.ipv6; > ub->ifindex = dev->ifindex; >+ b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr); > b->mtu = 1280; > #endif > } else { >-- >2.39.1 |
From: Xin L. <luc...@gm...> - 2023-05-03 13:40:53
|
On Tue, May 2, 2023 at 11:37 PM Tung Quang Nguyen <tun...@de...> wrote: > > >As different media may requires different min mtu, and even the > >same media with different net family requires different min mtu, > >add tipc_bearer_min_mtu() to calculate min mtu accordingly. > > > >This API will be used to check the new mtu when doing the link > >mtu negotiation in the next patch. > > > >v1->v2: > > - use bearer_get() to avoid the open code. > This version change comment does not seem right. Please correct it to avoid confusion and put it after "---". See the comment in Patch 2/3. > > > >Signed-off-by: Xin Long <luc...@gm...> > >--- > > net/tipc/bearer.c | 13 +++++++++++++ > > net/tipc/bearer.h | 3 +++ > > net/tipc/udp_media.c | 5 +++-- > > 3 files changed, 19 insertions(+), 2 deletions(-) > > > >diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c > >index 35cac7733fd3..0e9a29e1536b 100644 > >--- a/net/tipc/bearer.c > >+++ b/net/tipc/bearer.c > >@@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id) > > return mtu; > > } > > > >+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id) > >+{ > >+ int mtu = TIPC_MIN_BEARER_MTU; > >+ struct tipc_bearer *b; > >+ > >+ rcu_read_lock(); > >+ b = bearer_get(net, bearer_id); > >+ if (b) > >+ mtu += b->encap_hlen; > >+ rcu_read_unlock(); > >+ return mtu; > >+} > >+ > > /* tipc_bearer_xmit_skb - sends buffer to destination over bearer > > */ > > void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, > >diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h > >index 490ad6e5f7a3..bd0cc5c287ef 100644 > >--- a/net/tipc/bearer.h > >+++ b/net/tipc/bearer.h > >@@ -146,6 +146,7 @@ struct tipc_media { > > * @identity: array index of this bearer within TIPC bearer array > > * @disc: ptr to link setup request > > * @net_plane: network plane ('A' through 'H') currently associated with bearer > >+ * @encap_hlen: encap headers length > > * @up: bearer up flag (bit 0) > > * @refcnt: tipc_bearer reference counter > > * > >@@ -170,6 +171,7 @@ struct tipc_bearer { > > u32 identity; > > struct tipc_discoverer *disc; > > char net_plane; > >+ u16 encap_hlen; > > unsigned long up; > > refcount_t refcnt; > > }; > >@@ -232,6 +234,7 @@ int tipc_bearer_setup(void); > > void tipc_bearer_cleanup(void); > > void tipc_bearer_stop(struct net *net); > > int tipc_bearer_mtu(struct net *net, u32 bearer_id); > >+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id); > > bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id); > > void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id, > > struct sk_buff *skb, > >diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c > >index c2bb818704c8..0a85244fd618 100644 > >--- a/net/tipc/udp_media.c > >+++ b/net/tipc/udp_media.c > >@@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, > > udp_conf.local_ip.s_addr = local.ipv4.s_addr; > > udp_conf.use_udp_checksums = false; > > ub->ifindex = dev->ifindex; > >- if (tipc_mtu_bad(dev, sizeof(struct iphdr) + > >- sizeof(struct udphdr))) { > >+ b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr); > >+ if (tipc_mtu_bad(dev, b->encap_hlen)) { > I agree that calling tipc_mtu_bad() is not necessary for UDP bearer enabling. You can remove it in this patch. To be honest, it's NOT appropriate to do code cleanup in this patch, nor in net.git. Thanks. > > err = -EINVAL; > > goto err; > > } > >@@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, > > else > > udp_conf.local_ip6 = local.ipv6; > > ub->ifindex = dev->ifindex; > >+ b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr); > > b->mtu = 1280; > > #endif > > } else { > >-- > >2.39.1 > |
From: Xin L. <luc...@gm...> - 2023-05-03 13:35:31
|
On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen <tun...@de...> 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/201...@da.../ unless there are some new rules I missed. Thanks. > > > >Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation") > >Reported-by: Shuang Li <sh...@re...> > >Signed-off-by: Xin Long <luc...@gm...> > >--- > > 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 > |
From: Jon M. <jm...@re...> - 2023-05-03 19:29:25
|
On 2023-05-03 09:35, Xin Long wrote: > On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen > <tun...@de...> 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/201...@da.../ > > 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 <sh...@re...> >>> Signed-off-by: Xin Long <luc...@gm...> >>> --- >>> 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 |
From: Jon M. <jm...@re...> - 2023-05-07 21:46:45
|
On 2023-05-02 18:13, Xin Long wrote: > This patchset fixes a crash caused by a too small MTU carried in the > activate msg. Note that as such malicious packet does not exist in > the normal env, the fix won't break any application > > The 1st patch introduces a function to calculate the minimum MTU for > the bearer, and the 2nd patch fixes the crash with this helper. While > at it, the 3rd patch fixes the udp bearer mtu update by netlink with > this helper. > > Xin Long (3): > tipc: add tipc_bearer_min_mtu to calculate min mtu > tipc: do not update mtu if msg_max is too small in mtu negotiation > tipc: check the bearer min mtu properly when setting it by netlink > > net/tipc/bearer.c | 17 +++++++++++++++-- > net/tipc/bearer.h | 3 +++ > net/tipc/link.c | 9 ++++++--- > net/tipc/udp_media.c | 5 +++-- > 4 files changed, 27 insertions(+), 7 deletions(-) > Series Acked-by: Jon Maloy <jm...@re...> |