RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Thursday, 20 October, 2016 08:46
To: Ying Xue <redacted>; Jon Maloy <redacted>
Cc: netdev@vger.kernel.org; Qian Zhang <redacted>; Eric Dumazet
[off-list ref]
Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

On Thu, 2016-10-20 at 17:30 +0800, Ying Xue wrote:

quoted

On 10/19/2016 10:16 AM, Ben Hutchings wrote:

quoted

Qian Zhang (张谦) reported a potential socket buffer overflow in
tipc_msg_build().  The minimum fragment length needs to be checked
against the maximum packet size, which is based on the link MTU.

[...]

quoted

quoted

--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c

@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct

msghdr *m,

quoted

quoted

quoted

quoted

 		goto error;
 	}

quoted

quoted

+	/* Check that fragment and message header will fit */
+	if (INT_H_SIZE + mhsz > pktmax)

+		return -EMSGSIZE;

The "mhsz" represents the size of tipc packet header for current socket,
INT_H_SIZE indicates the size of tipc internal message header. So it
seems unreasonable to identify whether the sum of both header sizes is
bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to
compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should
return EMSGSIZE error code.

At this point we're about to copy INT_H_SIZE + mhsz bytes into the
first fragment.  If that's already limited to be less than or equal to
MAX_H_SIZE, comparing with MAX_H_SIZE would be fine.  But if MAX_H_SIZE
is the maximum value of mhsz, that won't be good enough.

MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger than the biggest header we are actually using, which is MCAST_H_SIZE (==44 bytes).
INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu < 84 bytes.
You won't find any interfaces or protocols that come even close to this limitation, so to me this test is redundant.

Regards
///jon

Ben.

quoted

quoted

+

quoted

quoted

 	/* Prepare reusable fragment header */
 	tipc_msg_init(msg_prevnode(mhdr), &pkthdr,

MSG_FRAGMENTER,

quoted

quoted

quoted

quoted

 		      FIRST_FRAGMENT, INT_H_SIZE,

msg_destnode(mhdr));

quoted

quoted

--
Ben Hutchings
Never put off till tomorrow what you can avoid all together.