Re: [tipc-discussion] [net] tipc: fix kernel WARNING in tipc_msg_append()

[Tuong T. L. <tuo...@de...>]

> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Monday, June 8, 2020 8:13 PM
> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
> dis...@li...
> Cc: tipc-dek <tip...@de...>
> Subject: Re: [net] tipc: fix kernel WARNING in tipc_msg_append()
> 
> 
> 
> On 6/8/20 8:05 AM, Tuong Lien wrote:
> > syzbot found the following issue:
> >
> > WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 check_copy_size include/linux/thread_info.h:150 [inline]
> > WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 copy_from_iter include/linux/uio.h:144 [inline]
> > WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 tipc_msg_append+0x49a/0x5e0 net/tipc/msg.c:242
> > Kernel panic - not syncing: panic_on_warn set ...
> >
> > This happens after commit 5e9eeccc58f3 ("tipc: fix NULL pointer
> > dereference in streaming") that tried to build at least one buffer even
> > when the message data length is zero... However, it now exposes another
> > bug that the 'mss' can be zero and the 'cpy' will be negative, thus the
> > above kernel WARNING will appear!
> > The zero value of 'mss' is never expected because it means Nagle is not
> > enabled for the socket (actually the socket type was 'SOCK_SEQPACKET'),
> > so the function 'tipc_msg_append()' must not be called at all. But that
> > was in this particular case since the message data length was zero, and
> > the 'send <= maxnagle' check became true.
> >
> > We resolve the issue by explicitly checking if Nagle is enabled for the
> > socket, i.e. 'maxnagle != 0' before calling the 'tipc_msg_append()'. In
> > addition, we put a sanity check in the function to avoid calling the
> > 'copy_from_iter()' with a negative size and doing an infinite loop.
> >
> > Reported-by: syz...@sy...
> > Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
> > Signed-off-by: Tuong Lien <tuo...@de...>
> > ---
> >   net/tipc/msg.c    | 5 +++--
> >   net/tipc/socket.c | 3 ++-
> >   2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> > index 046e4cb3acea..ea3ebf35e0c2 100644
> > --- a/net/tipc/msg.c
> > +++ b/net/tipc/msg.c
> > @@ -239,13 +239,14 @@ int tipc_msg_append(struct tipc_msg *_hdr, struct msghdr *m, int dlen,
> >   		curr = msg_blocks(hdr);
> >   		mlen = msg_size(hdr);
> >   		cpy = min_t(int, rem, mss - mlen);
> > -		if (cpy != copy_from_iter(skb->data + mlen, cpy, &m->msg_iter))
> > +		if (cpy < 0 ||
> You can probably just redeclare cpy (and mlen, rem) to u32 here.
> ///jon
Yes, it should be 'unsigned', but the actual issue here is overflow, so if we use u32, we will still need to check if not > INT_MAX... Instead, I think we can just change the data type at the 'min_t()', such as:
	cpy = min_t(unsinged int, rem, mss - mlen);
Do you agree?

> > +		    cpy != copy_from_iter(skb->data + mlen, cpy, &m->msg_iter))
> >   			return -EFAULT;
> >   		msg_set_size(hdr, mlen + cpy);
> >   		skb_put(skb, cpy);
> >   		rem -= cpy;
> >   		total += msg_blocks(hdr) - curr;
> > -	} while (rem);
> > +	} while (rem > 0);
> >   	return total - accounted;
> >   }
> >
> > diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> > index 26123f4177fd..a94f38333698 100644
> > --- a/net/tipc/socket.c
> > +++ b/net/tipc/socket.c
> > @@ -1574,7 +1574,8 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
> >   			break;
> >   		send = min_t(size_t, dlen - sent, TIPC_MAX_USER_MSG_SIZE);
> >   		blocks = tsk->snd_backlog;
> > -		if (tsk->oneway++ >= tsk->nagle_start && send <= maxnagle) {
> > +		if (tsk->oneway++ >= tsk->nagle_start && maxnagle &&
> > +		    send <= maxnagle) {
How about this? I believe this is a must because we never want to do Nagle stuffs for a non-Nagle socket (like SOCK_SEQPACKET).

> >   			rc = tipc_msg_append(hdr, m, send, maxnagle, txq);
> >   			if (unlikely(rc < 0))
> >   				break;