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