> -----Original Message----- > From: Ben Hutchings [mailto:b...@decadent.org.uk] > Sent: Thursday, 20 October, 2016 08:46 > To: Ying Xue <ying.x...@gmail.com>; Jon Maloy <jon.ma...@ericsson.com> > Cc: netdev@vger.kernel.org; Qian Zhang <zhangqia...@360.cn>; Eric Dumazet > <eduma...@google.com> > 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: > > On 10/19/2016 10:16 AM, Ben Hutchings wrote: > > > 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. > [...] > > > > > > --- 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, > > > > > goto error; > > > > > } > > > > > > > > + /* 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. > > > > + > > > > > /* 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.
