Message-ID: <e10431d6-a768-121a-be53-6674c63dbceb@windriver.com>
Date:   Wed, 23 May 2018 21:38:33 +0800
From:   Ying Xue <ying.xue@...driver.com>
To:     David Miller <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>, <jon.maloy@...csson.com>,
        <syzkaller-bugs@...glegroups.com>,
        <tipc-discussion@...ts.sourceforge.net>, <dvyukov@...gle.com>
Subject: Re: [PATCH net-next] tipc: eliminate complaint of KMSAN uninit-value
 in tipc_conn_rcv_sub

On 05/20/2018 11:00 AM, David Miller wrote:
> From: Ying Xue <ying.xue@...driver.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.

Powered by blists - more mailing lists