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

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

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

--- 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

 		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.

Ben.

quoted

+

quoted

quoted

 	/* Prepare reusable fragment header */
 	tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER,
 		      FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr));

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