[PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message

[PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message

[Tung Nguyen]

When sending a SYN message, this kernel stack trace is observed:

...
[   13.396352] RIP: 0010:_copy_from_iter+0xb4/0x550
...
[   13.398494] Call Trace:
[   13.398630]  <TASK>
[   13.398630]  ? __alloc_skb+0xed/0x1a0
[   13.398630]  tipc_msg_build+0x12c/0x670 [tipc]
[   13.398630]  ? shmem_add_to_page_cache.isra.71+0x151/0x290
[   13.398630]  __tipc_sendmsg+0x2d1/0x710 [tipc]
[   13.398630]  ? tipc_connect+0x1d9/0x230 [tipc]
[   13.398630]  ? __local_bh_enable_ip+0x37/0x80
[   13.398630]  tipc_connect+0x1d9/0x230 [tipc]
[   13.398630]  ? __sys_connect+0x9f/0xd0
[   13.398630]  __sys_connect+0x9f/0xd0
[   13.398630]  ? preempt_count_add+0x4d/0xa0
[   13.398630]  ? fpregs_assert_state_consistent+0x22/0x50
[   13.398630]  __x64_sys_connect+0x16/0x20
[   13.398630]  do_syscall_64+0x42/0x90
[   13.398630]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

It is because commit a41dad905e5a ("iov_iter: saner checks for attempt
to copy to/from iterator") has introduced sanity check for copying
from/to iov iterator. Lacking of copy direction from the iterator
viewpoint would lead to kernel stack trace like above.

This commit fixes this issue by initializing the iov iterator with
the correct copy direction.

Fixes: f25dcc7687d4 ("tipc: tipc ->sendmsg() conversion")
Reported-by: syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com
Acked-by: Jon Maloy <jmaloy@redhat.com>
Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
---
v2: add Fixes tag

 net/tipc/msg.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 5c9fd4791c4b..cce118fea07a 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -381,6 +381,9 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, int offset,
 
 	msg_set_size(mhdr, msz);
 
+	if (!dsz)
+		iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0);
+
 	/* No fragmentation needed? */
 	if (likely(msz <= pktmax)) {
 		skb = tipc_buf_acquire(msz, GFP_KERNEL);
-- 
2.34.1

Re: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message

[Paolo Abeni]

On Wed, 2023-02-08 at 07:07 +0000, Tung Nguyen wrote:
> When sending a SYN message, this kernel stack trace is observed:
> 
> ...
> [   13.396352] RIP: 0010:_copy_from_iter+0xb4/0x550
> ...
> [   13.398494] Call Trace:
> [   13.398630]  <TASK>
> [   13.398630]  ? __alloc_skb+0xed/0x1a0
> [   13.398630]  tipc_msg_build+0x12c/0x670 [tipc]
> [   13.398630]  ? shmem_add_to_page_cache.isra.71+0x151/0x290
> [   13.398630]  __tipc_sendmsg+0x2d1/0x710 [tipc]
> [   13.398630]  ? tipc_connect+0x1d9/0x230 [tipc]
> [   13.398630]  ? __local_bh_enable_ip+0x37/0x80
> [   13.398630]  tipc_connect+0x1d9/0x230 [tipc]
> [   13.398630]  ? __sys_connect+0x9f/0xd0
> [   13.398630]  __sys_connect+0x9f/0xd0
> [   13.398630]  ? preempt_count_add+0x4d/0xa0
> [   13.398630]  ? fpregs_assert_state_consistent+0x22/0x50
> [   13.398630]  __x64_sys_connect+0x16/0x20
> [   13.398630]  do_syscall_64+0x42/0x90
> [   13.398630]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> It is because commit a41dad905e5a ("iov_iter: saner checks for attempt
> to copy to/from iterator") has introduced sanity check for copying
> from/to iov iterator. Lacking of copy direction from the iterator
> viewpoint would lead to kernel stack trace like above.
> 
> This commit fixes this issue by initializing the iov iterator with
> the correct copy direction.
> 
> Fixes: f25dcc7687d4 ("tipc: tipc ->sendmsg() conversion")
> Reported-by: syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com
> Acked-by: Jon Maloy <jmaloy@redhat.com>
> Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
> ---
> v2: add Fixes tag
> 
>  net/tipc/msg.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 5c9fd4791c4b..cce118fea07a 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -381,6 +381,9 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, int offset,
>  
>  	msg_set_size(mhdr, msz);
>  
> +	if (!dsz)
> +		iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0);

It looks like the root cause of the problem is that not all (indirect)
callers of tipc_msg_build() properly initialize the iter.

tipc_connect() is one of such caller, but AFAICS even tipc_accept() can
reach tipc_msg_build() without proper iter initialization - via
__tipc_sendstream -> __tipc_sendmsg.

I think it's better if you address the issue in relevant callers,
avoiding unneeded and confusing code in tipc_msg_build().

Thanks,

Paolo

RE: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message

[Tung Quang Nguyen]

>-----Original Message-----
>From: Paolo Abeni <pabeni@redhat.com>
>Sent: Thursday, February 9, 2023 5:39 PM
>To: Tung Quang Nguyen <tung.q.nguyen@dektech.com.au>; netdev@vger.kernel.org
>Cc: davem@davemloft.net; kuba@kernel.org; edumazet@google.com; jmaloy@redhat.com; ying.xue@windriver.com;
>viro@zeniv.linux.org.uk; syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com
>Subject: Re: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message
>
>On Wed, 2023-02-08 at 07:07 +0000, Tung Nguyen wrote:
>> When sending a SYN message, this kernel stack trace is observed:
>>
>> ...
>> [   13.396352] RIP: 0010:_copy_from_iter+0xb4/0x550
>> ...
>> [   13.398494] Call Trace:
>> [   13.398630]  <TASK>
>> [   13.398630]  ? __alloc_skb+0xed/0x1a0
>> [   13.398630]  tipc_msg_build+0x12c/0x670 [tipc]
>> [   13.398630]  ? shmem_add_to_page_cache.isra.71+0x151/0x290
>> [   13.398630]  __tipc_sendmsg+0x2d1/0x710 [tipc]
>> [   13.398630]  ? tipc_connect+0x1d9/0x230 [tipc]
>> [   13.398630]  ? __local_bh_enable_ip+0x37/0x80
>> [   13.398630]  tipc_connect+0x1d9/0x230 [tipc]
>> [   13.398630]  ? __sys_connect+0x9f/0xd0
>> [   13.398630]  __sys_connect+0x9f/0xd0
>> [   13.398630]  ? preempt_count_add+0x4d/0xa0
>> [   13.398630]  ? fpregs_assert_state_consistent+0x22/0x50
>> [   13.398630]  __x64_sys_connect+0x16/0x20
>> [   13.398630]  do_syscall_64+0x42/0x90
>> [   13.398630]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> It is because commit a41dad905e5a ("iov_iter: saner checks for attempt
>> to copy to/from iterator") has introduced sanity check for copying
>> from/to iov iterator. Lacking of copy direction from the iterator
>> viewpoint would lead to kernel stack trace like above.
>>
>> This commit fixes this issue by initializing the iov iterator with
>> the correct copy direction.
>>
>> Fixes: f25dcc7687d4 ("tipc: tipc ->sendmsg() conversion")
>> Reported-by: syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com
>> Acked-by: Jon Maloy <jmaloy@redhat.com>
>> Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
>> ---
>> v2: add Fixes tag
>>
>>  net/tipc/msg.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
>> index 5c9fd4791c4b..cce118fea07a 100644
>> --- a/net/tipc/msg.c
>> +++ b/net/tipc/msg.c
>> @@ -381,6 +381,9 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, int offset,
>>
>>  	msg_set_size(mhdr, msz);
>>
>> +	if (!dsz)
>> +		iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0);
>
>It looks like the root cause of the problem is that not all (indirect)
>callers of tipc_msg_build() properly initialize the iter.
>
>tipc_connect() is one of such caller, but AFAICS even tipc_accept() can
>reach tipc_msg_build() without proper iter initialization - via
>__tipc_sendstream -> __tipc_sendmsg.
>
>I think it's better if you address the issue in relevant callers,
>avoiding unneeded and confusing code in tipc_msg_build().
I am fully aware of callers (without initializing iovec) of this function. My intention was to make as less change as possible.
Do you think using  iov_iter_kvec() instead in the callers make sense if I go for what you suggested ?
>
>Thanks,
>
>Paolo

Re: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message

[Jakub Kicinski]

On Thu, 9 Feb 2023 11:10:16 +0000 Tung Quang Nguyen wrote:
> >>  	msg_set_size(mhdr, msz);
> >>
> >> +	if (!dsz)
> >> +		iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0);  
> >
> >It looks like the root cause of the problem is that not all (indirect)
> >callers of tipc_msg_build() properly initialize the iter.
> >
> >tipc_connect() is one of such caller, but AFAICS even tipc_accept() can
> >reach tipc_msg_build() without proper iter initialization - via
> >__tipc_sendstream -> __tipc_sendmsg.
> >
> >I think it's better if you address the issue in relevant callers,
> >avoiding unneeded and confusing code in tipc_msg_build().  
>
> I am fully aware of callers (without initializing iovec) of this
> function. My intention was to make as less change as possible.

General kernel guidance is to fix things "right" (i.e. so that the fix
doesn't have to be refactored later). 

> Do you think using  iov_iter_kvec() instead in the callers make sense
> if I go for what you suggested ?

I think so. These are the potential culprits?

$ git grep 'struct msghdr [^*]*;' -- net/tipc/
net/tipc/socket.c:      struct msghdr m = {NULL,};
net/tipc/socket.c:      struct msghdr m = {NULL,};
net/tipc/topsrv.c:      struct msghdr msg;
net/tipc/topsrv.c:      struct msghdr msg = {};

RE: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message

[Tung Quang Nguyen]

>-----Original Message-----
>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Saturday, February 11, 2023 10:16 AM
>To: Tung Quang Nguyen <tung.q.nguyen@dektech.com.au>
>Cc: Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com;
>jmaloy@redhat.com; ying.xue@windriver.com; viro@zeniv.linux.org.uk; syzbot+d43608d061e8847ec9f3@syzkaller.appspotmail.com
>Subject: Re: [PATCH v2 net 1/1] tipc: fix kernel warning when sending SYN message
>
>On Thu, 9 Feb 2023 11:10:16 +0000 Tung Quang Nguyen wrote:
>> >>  	msg_set_size(mhdr, msz);
>> >>
>> >> +	if (!dsz)
>> >> +		iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0);
>> >
>> >It looks like the root cause of the problem is that not all (indirect)
>> >callers of tipc_msg_build() properly initialize the iter.
>> >
>> >tipc_connect() is one of such caller, but AFAICS even tipc_accept() can
>> >reach tipc_msg_build() without proper iter initialization - via
>> >__tipc_sendstream -> __tipc_sendmsg.
>> >
>> >I think it's better if you address the issue in relevant callers,
>> >avoiding unneeded and confusing code in tipc_msg_build().
>>
>> I am fully aware of callers (without initializing iovec) of this
>> function. My intention was to make as less change as possible.
>
>General kernel guidance is to fix things "right" (i.e. so that the fix
>doesn't have to be refactored later).
>
>> Do you think using  iov_iter_kvec() instead in the callers make sense
>> if I go for what you suggested ?
>
>I think so. These are the potential culprits?
>
>$ git grep 'struct msghdr [^*]*;' -- net/tipc/
>net/tipc/socket.c:      struct msghdr m = {NULL,};
>net/tipc/socket.c:      struct msghdr m = {NULL,};
>net/tipc/topsrv.c:      struct msghdr msg;
>net/tipc/topsrv.c:      struct msghdr msg = {};
Thanks. I will send v3.