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. 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.
signature.asc
Description: This is a digitally signed message part
