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

[Jon M. <jm...@re...>]

On 6/8/20 11:55 PM, 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.
---
Same suggestion as I had to Hoang; add the three dashes above to avoid that
the version info by accident becomes part of the commit log.
>
> v2: use 'size_t' in the 'min_t()' to get a proper value of 'cpy' (after
>      Jon's comment)
>
> Reported-by: syz...@sy...
> Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
> Signed-off-by: Tuong Lien <tuo...@de...>
> ---
>   net/tipc/msg.c    | 4 ++--
>   net/tipc/socket.c | 3 ++-
>   2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 046e4cb3acea..01b64869a173 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -238,14 +238,14 @@ int tipc_msg_append(struct tipc_msg *_hdr, struct msghdr *m, int dlen,
>   		hdr = buf_msg(skb);
>   		curr = msg_blocks(hdr);
>   		mlen = msg_size(hdr);
> -		cpy = min_t(int, rem, mss - mlen);
> +		cpy = min_t(size_t, rem, mss - mlen);
>   		if (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) {
>   			rc = tipc_msg_append(hdr, m, send, maxnagle, txq);
>   			if (unlikely(rc < 0))
>   				break;
Acked-by: Jon Maloy <jm...@re...>