diff mbox series

[PATCHv2,net,2/3] tipc: do not update mtu if msg_max is too small in mtu negotiation

Message ID 0d328807d5087d0b6d03c3d2e5f355cd44ed576a.1683065352.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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers fail 1 blamed authors not CCed: ying.xue@windriver.com; 1 maintainers not CCed: ying.xue@windriver.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 27 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Xin Long May 2, 2023, 10:13 p.m. UTC 
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 <shuali@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/tipc/link.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Tung Quang Nguyen May 3, 2023, 3:31 a.m. UTC | #1 
>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 <shuali@redhat.com>
>Signed-off-by: Xin Long <lucien.xin@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
Xin Long May 3, 2023, 1:35 p.m. UTC | #2 
On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen
<tung.q.nguyen@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.davem@davemloft.net/

unless there are some new rules I missed.

Thanks.

> >
> >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 | 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
>
Jon Maloy May 3, 2023, 7:29 p.m. UTC | #3 
On 2023-05-03 09:35, Xin Long wrote:
> On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen
> <tung.q.nguyen@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.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@redhat.com>
>>> Signed-off-by: Xin Long <lucien.xin@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
Jakub Kicinski May 4, 2023, 2:55 a.m. UTC | #4 
On Wed, 3 May 2023 15:29:07 -0400 Jon Maloy wrote:
> > 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.

Yes, I think the motivation has changed a bit since we now have 
the permanent lore archive and we add links when applying patches.
The change log is easy to find on lore, even after the --- delimiter.
diff mbox series

Patch

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: