> -----Original Message-----
> From: Jon Maloy <jma...@redhat.com>
> Sent: Monday, June 8, 2020 8:13 PM
> To: Tuong Tong Lien <tuong.t.l...@dektech.com.au>; ma...@donjonn.com;
> ying....@windriver.com; tipc-
> discuss...@lists.sourceforge.net
> Cc: tipc-dek <tipc-...@dektech.com.au>
> 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: syzbot+75139a7d2605236b0...@syzkaller.appspotmail.com
> > Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
> > Signed-off-by: Tuong Lien <tuong.t.l...@dektech.com.au>
> > ---
> > 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;
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
