From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ying Xue Subject: Re: [PATCH net-next] tipc: eliminate complaint of KMSAN uninit-value in tipc_conn_rcv_sub Date: Wed, 23 May 2018 21:38:33 +0800 Message-ID: References: <1526644255-9182-1-git-send-email-ying.xue@windriver.com> <20180519.230021.538446373514892322.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, syzkaller-bugs@googlegroups.com, tipc-discussion@lists.sourceforge.net, dvyukov@google.com To: David Miller Return-path: In-Reply-To: <20180519.230021.538446373514892322.davem@davemloft.net> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tipc-discussion-bounces@lists.sourceforge.net List-Id: netdev.vger.kernel.org On 05/20/2018 11:00 AM, David Miller wrote: > From: Ying Xue > 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. ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot