[net-next] tipc: eliminate complaint of KMSAN uninit-value in tipc_conn_rcv_sub

As variable s of struct tipc_subscr type is not initialized
in tipc_conn_rcv_from_sock() before it is used in tipc_conn_rcv_sub(),
KMSAN reported the following uninit-value type complaint:

On Fri, May 18, 2018 at 1:50 PM, Ying Xue <ying.xue@windriver.com> wrote:
> As variable s of struct tipc_subscr type is not initialized
> in tipc_conn_rcv_from_sock() before it is used in tipc_conn_rcv_sub(),
> KMSAN reported the following uninit-value type complaint:
>
> ==================================================================
> BUG: KMSAN: uninit-value in tipc_conn_rcv_sub+0x184/0x950
> net/tipc/topsrv.c:373
> CPU: 0 PID: 66 Comm: kworker/u4:4 Not tainted 4.17.0-rc3+ #88
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Workqueue: tipc_rcv tipc_conn_recv_work
> Call Trace:
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x185/0x1d0 lib/dump_stack.c:113
>   kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
>   __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
>   tipc_conn_rcv_sub+0x184/0x950 net/tipc/topsrv.c:373
>   tipc_conn_rcv_from_sock net/tipc/topsrv.c:409 [inline]
>   tipc_conn_recv_work+0x3cd/0x560 net/tipc/topsrv.c:424
>   process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
>   worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279
>   kthread+0x539/0x720 kernel/kthread.c:239
>   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412
>
> Local variable description: ----s.i@tipc_conn_recv_work
> Variable was created at:
>   tipc_conn_recv_work+0x65/0x560 net/tipc/topsrv.c:419
>   process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
> ==================================================================
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 0 PID: 66 Comm: kworker/u4:4 Tainted: G    B             4.17.0-rc3+
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Workqueue: tipc_rcv tipc_conn_recv_work
> Call Trace:
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x185/0x1d0 lib/dump_stack.c:113
>   panic+0x39d/0x940 kernel/panic.c:184
>   kmsan_report+0x238/0x240 mm/kmsan/kmsan.c:1083
>   __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
>   tipc_conn_rcv_sub+0x184/0x950 net/tipc/topsrv.c:373
>   tipc_conn_rcv_from_sock net/tipc/topsrv.c:409 [inline]
>   tipc_conn_recv_work+0x3cd/0x560 net/tipc/topsrv.c:424
>   process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
>   worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279
>   kthread+0x539/0x720 kernel/kthread.c:239
>   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412
> Dumping ftrace buffer:
>     (ftrace buffer empty)
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
>
> Reported-by: syzbot+8951a3065ee7fd6d6e23@syzkaller.appspotmail.com
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> ---
>  net/tipc/topsrv.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index c8e34ef..fe47a62 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -397,6 +397,7 @@ static int tipc_conn_rcv_from_sock(struct tipc_conn *con)
>         struct kvec iov;
>         int ret;
>
> +       memset(&s, 0, sizeof(s));
>         iov.iov_base = &s;
>         iov.iov_len = sizeof(s);
>         msg.msg_name = NULL;


Isn't the kernel bug here that sock_recvmsg does a short read? It
seems that sock_recvmsg should initialize all of s.

From: Ying Xue <ying.xue@windriver.com>
Date: Fri, 18 May 2018 19:50:55 +0800

> As variable s of struct tipc_subscr type is not initialized
> in tipc_conn_rcv_from_sock() before it is used in tipc_conn_rcv_sub(),
> KMSAN reported the following uninit-value type complaint:

I agree with others that the short read is the bug.

You need to decide what should happen if not a full tipc_subscr object
is obtained from the sock_recvmsg() call.

Proceeding to pass it on to tipc_conn_rcv_sub() cannot possibly be
correct.

You're not getting what you are expecting from the peer, the memset()
you are adding doesn't change that.

And once you get this badly sized read, what does that do to
the stream of subsequent recvmsg calls here?

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org>
> On Behalf Of David Miller
> Sent: Saturday, May 19, 2018 23:00
> To: ying.xue@windriver.com
> Cc: netdev@vger.kernel.org; Jon Maloy <jon.maloy@ericsson.com>;
> syzkaller-bugs@googlegroups.com; tipc-discussion@lists.sourceforge.net
> Subject: Re: [PATCH net-next] tipc: eliminate complaint of KMSAN uninit-
> value in tipc_conn_rcv_sub
> 
> From: Ying Xue <ying.xue@windriver.com>
> Date: Fri, 18 May 2018 19:50:55 +0800
> 
> > As variable s of struct tipc_subscr type is not initialized in
> > tipc_conn_rcv_from_sock() before it is used in tipc_conn_rcv_sub(),
> > KMSAN reported the following uninit-value type complaint:
> 
> I agree with others that the short read is the bug.
> 
> You need to decide what should happen if not a full tipc_subscr object is
> obtained from the sock_recvmsg() call.
> 
> Proceeding to pass it on to tipc_conn_rcv_sub() cannot possibly be correct.
> 
> You're not getting what you are expecting from the peer, the memset() you
> are adding doesn't change that.
> 
> And once you get this badly sized read, what does that do to the stream of
> subsequent recvmsg calls here?

This socket/connection of type SOCK_SEQPACKET, so if anything like this happens, it is an error, and the connection should be aborted.
///jon

On 05/20/2018 11:00 AM, David Miller wrote:
> From: Ying Xue <ying.xue@windriver.com>
> Date: Fri, 18 May 2018 19:50:55 +0800
> 
>> As variable s of struct tipc_subscr type is not initialized
>> in tipc_conn_rcv_from_sock() before it is used in tipc_conn_rcv_sub(),
>> KMSAN reported the following uninit-value type complaint:
> 
> I agree with others that the short read is the bug.

In this error case, Dmitry is right. A short read happened in
tipc_recvmsg() especially when the size of skb received from a socket of
user space was smaller than the msg_iter.count of struct msghdr (ie,
tipc_subscr object size) passed by tipc_conn_rcv_from_sock() in kernel
space.

But when tipc_recvmsg() copied the data of skb to msg_iter.kvec of
struct msghdr with skb data length rather than msg_iter.count, it means
the part of space (ie, msg_iter.count - skb data length) of
msg_iter.kvec was not initialized. For the detailed info, please refer
to its relevant code:

tipc_recvmsg()
{
...
        if (likely(!err)) {
                copy = min_t(int, dlen, buflen);
                if (unlikely(copy != dlen))
                        m->msg_flags |= MSG_TRUNC;
                rc = skb_copy_datagram_msg(skb, hlen, m, copy);
...
}

If we receive a skb message with recvmsg() in user space, it seems no
problem even if the length of msg_iter.iov is bigger than skb data size.
But under the same situation, if we receive a message through
sock_recvmsg() in kernel space, it might be a problem.

I have checked the receive functions of other stacks like TCP and UDP,
as a result, when msg_iter.count is bigger than skb->len, they never use
memset() to initialize the remaining area of msg_iter.kvec (ie,
msg_iter.count - skb->len) no matter whether we receive a message
through sock_recvmsg() in user space or kernel space.

> 
> You need to decide what should happen if not a full tipc_subscr object
> is obtained from the sock_recvmsg() call.
> 
> Proceeding to pass it on to tipc_conn_rcv_sub() cannot possibly be
> correct.

If we do not conduct a full read for tipc_subscr object through
sock_recvmsg() call, some fields of tipc_subscr object might be
incorrect. But I can confirm that the incorrect fields of tipc_subscr
object any fatal error except that we might wrongly add one subscription.

> 
> You're not getting what you are expecting from the peer, the memset()
> you are adding doesn't change that.

Before tipc_subscr object address is passed to msg_iter.kvec pointer, we
have initialized the whole tipc_subscr object with memset(). Just in
this case, this method should kill the uninit-value complaint.

If we initialize tipc_subscr object in tipc_recvmsg(), we have to
conduct initialization behavior for the msg_iter.kvec/msg_iter.vio area
(msg_iter.count - skb->len) no matter whether a message is received from
user space or kernel space. Particularly when the caller of recvmsg() is
in user space, the initialization seems no necessary.

> 
> And once you get this badly sized read, what does that do to
> the stream of subsequent recvmsg calls here?
> 

Even if we get a bad sized read for tipc_subscr object in
tipc_conn_rcv_sub(), it doesn't cause any fatal impact on system or TIPC
stack.