diff mbox series

[net] net: Return error from sk_stream_wait_connect() if sk_wait_event() fails

Message ID 20231214050922.3480023-1-syoshida@redhat.com
State New
Headers
Series [net] net: Return error from sk_stream_wait_connect() if sk_wait_event() fails |

Commit Message

Shigeru Yoshida Dec. 14, 2023, 5:09 a.m. UTC 
  The following NULL pointer dereference issue occurred:

BUG: kernel NULL pointer dereference, address: 0000000000000000
<...>
RIP: 0010:ccid_hc_tx_send_packet net/dccp/ccid.h:166 [inline]
RIP: 0010:dccp_write_xmit+0x49/0x140 net/dccp/output.c:356
<...>
Call Trace:
 <TASK>
 dccp_sendmsg+0x642/0x7e0 net/dccp/proto.c:801
 inet_sendmsg+0x63/0x90 net/ipv4/af_inet.c:846
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg+0x83/0xe0 net/socket.c:745
 ____sys_sendmsg+0x443/0x510 net/socket.c:2558
 ___sys_sendmsg+0xe5/0x150 net/socket.c:2612
 __sys_sendmsg+0xa6/0x120 net/socket.c:2641
 __do_sys_sendmsg net/socket.c:2650 [inline]
 __se_sys_sendmsg net/socket.c:2648 [inline]
 __x64_sys_sendmsg+0x45/0x50 net/socket.c:2648
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x43/0x110 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

sk_wait_event() returns an error (-EPIPE) if disconnect() is called on the
socket waiting for the event. However, sk_stream_wait_connect() returns
success, i.e. zero, even if sk_wait_event() returns -EPIPE, so a function
that waits for a connection with sk_stream_wait_connect() may misbehave.

In the case of the above DCCP issue, dccp_sendmsg() is waiting for the
connection. If disconnect() is called in concurrently, the above issue
occurs.

This patch fixes the issue by returning error from sk_stream_wait_connect()
if sk_wait_event() fails.

Fixes: 419ce133ab92 ("tcp: allow again tcp_disconnect() when threads are waiting")
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 net/core/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kuniyuki Iwashima Dec. 14, 2023, 8:46 a.m. UTC | #1 
From: Shigeru Yoshida <syoshida@redhat.com>
Date: Thu, 14 Dec 2023 14:09:22 +0900
> The following NULL pointer dereference issue occurred:
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> <...>
> RIP: 0010:ccid_hc_tx_send_packet net/dccp/ccid.h:166 [inline]
> RIP: 0010:dccp_write_xmit+0x49/0x140 net/dccp/output.c:356
> <...>
> Call Trace:
>  <TASK>
>  dccp_sendmsg+0x642/0x7e0 net/dccp/proto.c:801
>  inet_sendmsg+0x63/0x90 net/ipv4/af_inet.c:846
>  sock_sendmsg_nosec net/socket.c:730 [inline]
>  __sock_sendmsg+0x83/0xe0 net/socket.c:745
>  ____sys_sendmsg+0x443/0x510 net/socket.c:2558
>  ___sys_sendmsg+0xe5/0x150 net/socket.c:2612
>  __sys_sendmsg+0xa6/0x120 net/socket.c:2641
>  __do_sys_sendmsg net/socket.c:2650 [inline]
>  __se_sys_sendmsg net/socket.c:2648 [inline]
>  __x64_sys_sendmsg+0x45/0x50 net/socket.c:2648
>  do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>  do_syscall_64+0x43/0x110 arch/x86/entry/common.c:82
>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> sk_wait_event() returns an error (-EPIPE) if disconnect() is called on the
> socket waiting for the event. However, sk_stream_wait_connect() returns
> success, i.e. zero, even if sk_wait_event() returns -EPIPE, so a function
> that waits for a connection with sk_stream_wait_connect() may misbehave.
> 
> In the case of the above DCCP issue, dccp_sendmsg() is waiting for the
> connection. If disconnect() is called in concurrently, the above issue
> occurs.
> 
> This patch fixes the issue by returning error from sk_stream_wait_connect()
> if sk_wait_event() fails.
> 
> Fixes: 419ce133ab92 ("tcp: allow again tcp_disconnect() when threads are waiting")
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

I guess you picked this issue from syzbot's report.
https://lore.kernel.org/netdev/0000000000009e122006088a2b8d@google.com/

If so, let's give a proper credit to syzbot and its authors:

Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com

Thanks!

> ---
>  net/core/stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/stream.c b/net/core/stream.c
> index 96fbcb9bbb30..b16dfa568a2d 100644
> --- a/net/core/stream.c
> +++ b/net/core/stream.c
> @@ -79,7 +79,7 @@ int sk_stream_wait_connect(struct sock *sk, long *timeo_p)
>  		remove_wait_queue(sk_sleep(sk), &wait);
>  		sk->sk_write_pending--;
>  	} while (!done);
> -	return 0;
> +	return done < 0 ? done : 0;
>  }
>  EXPORT_SYMBOL(sk_stream_wait_connect);
>  
> -- 
> 2.41.0
>
Eric Dumazet Dec. 14, 2023, 10:56 a.m. UTC | #2 
On 12/14/23 06:09, Shigeru Yoshida wrote:
> The following NULL pointer dereference issue occurred:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> <...>
> RIP: 0010:ccid_hc_tx_send_packet net/dccp/ccid.h:166 [inline]
> RIP: 0010:dccp_write_xmit+0x49/0x140 net/dccp/output.c:356
> <...>
> Call Trace:
>   <TASK>
>   dccp_sendmsg+0x642/0x7e0 net/dccp/proto.c:801
>   inet_sendmsg+0x63/0x90 net/ipv4/af_inet.c:846
>   sock_sendmsg_nosec net/socket.c:730 [inline]
>   __sock_sendmsg+0x83/0xe0 net/socket.c:745
>   ____sys_sendmsg+0x443/0x510 net/socket.c:2558
>   ___sys_sendmsg+0xe5/0x150 net/socket.c:2612
>   __sys_sendmsg+0xa6/0x120 net/socket.c:2641
>   __do_sys_sendmsg net/socket.c:2650 [inline]
>   __se_sys_sendmsg net/socket.c:2648 [inline]
>   __x64_sys_sendmsg+0x45/0x50 net/socket.c:2648
>   do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>   do_syscall_64+0x43/0x110 arch/x86/entry/common.c:82
>   entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> sk_wait_event() returns an error (-EPIPE) if disconnect() is called on the
> socket waiting for the event. However, sk_stream_wait_connect() returns
> success, i.e. zero, even if sk_wait_event() returns -EPIPE, so a function
> that waits for a connection with sk_stream_wait_connect() may misbehave.
>
> In the case of the above DCCP issue, dccp_sendmsg() is waiting for the
> connection. If disconnect() is called in concurrently, the above issue
> occurs.
>
> This patch fixes the issue by returning error from sk_stream_wait_connect()
> if sk_wait_event() fails.
>
> Fixes: 419ce133ab92 ("tcp: allow again tcp_disconnect() when threads are waiting")
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>
Shigeru Yoshida Dec. 14, 2023, 1:31 p.m. UTC | #3 
On Thu, 14 Dec 2023 17:46:22 +0900, Kuniyuki Iwashima wrote:
> From: Shigeru Yoshida <syoshida@redhat.com>
> Date: Thu, 14 Dec 2023 14:09:22 +0900
>> The following NULL pointer dereference issue occurred:
>> 
>> BUG: kernel NULL pointer dereference, address: 0000000000000000
>> <...>
>> RIP: 0010:ccid_hc_tx_send_packet net/dccp/ccid.h:166 [inline]
>> RIP: 0010:dccp_write_xmit+0x49/0x140 net/dccp/output.c:356
>> <...>
>> Call Trace:
>>  <TASK>
>>  dccp_sendmsg+0x642/0x7e0 net/dccp/proto.c:801
>>  inet_sendmsg+0x63/0x90 net/ipv4/af_inet.c:846
>>  sock_sendmsg_nosec net/socket.c:730 [inline]
>>  __sock_sendmsg+0x83/0xe0 net/socket.c:745
>>  ____sys_sendmsg+0x443/0x510 net/socket.c:2558
>>  ___sys_sendmsg+0xe5/0x150 net/socket.c:2612
>>  __sys_sendmsg+0xa6/0x120 net/socket.c:2641
>>  __do_sys_sendmsg net/socket.c:2650 [inline]
>>  __se_sys_sendmsg net/socket.c:2648 [inline]
>>  __x64_sys_sendmsg+0x45/0x50 net/socket.c:2648
>>  do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>>  do_syscall_64+0x43/0x110 arch/x86/entry/common.c:82
>>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
>> 
>> sk_wait_event() returns an error (-EPIPE) if disconnect() is called on the
>> socket waiting for the event. However, sk_stream_wait_connect() returns
>> success, i.e. zero, even if sk_wait_event() returns -EPIPE, so a function
>> that waits for a connection with sk_stream_wait_connect() may misbehave.
>> 
>> In the case of the above DCCP issue, dccp_sendmsg() is waiting for the
>> connection. If disconnect() is called in concurrently, the above issue
>> occurs.
>> 
>> This patch fixes the issue by returning error from sk_stream_wait_connect()
>> if sk_wait_event() fails.
>> 
>> Fixes: 419ce133ab92 ("tcp: allow again tcp_disconnect() when threads are waiting")
>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> 
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> 
> I guess you picked this issue from syzbot's report.
> https://lore.kernel.org/netdev/0000000000009e122006088a2b8d@google.com/
> 
> If so, let's give a proper credit to syzbot and its authors:
> 
> Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com

Hi Kuniyuki-san,

Thank you very much for your review. I didn't notice the syzbot's
report. Actually, I found this issue by running syzkaller on my
machine.

Now, I tested this patch with syzbot, and it looks good.

Reported-and-tested-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com

Thanks,
Shigeru

> 
> Thanks!
> 
>> ---
>>  net/core/stream.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/core/stream.c b/net/core/stream.c
>> index 96fbcb9bbb30..b16dfa568a2d 100644
>> --- a/net/core/stream.c
>> +++ b/net/core/stream.c
>> @@ -79,7 +79,7 @@ int sk_stream_wait_connect(struct sock *sk, long *timeo_p)
>>  		remove_wait_queue(sk_sleep(sk), &wait);
>>  		sk->sk_write_pending--;
>>  	} while (!done);
>> -	return 0;
>> +	return done < 0 ? done : 0;
>>  }
>>  EXPORT_SYMBOL(sk_stream_wait_connect);
>>  
>> -- 
>> 2.41.0
>> 
>
Kuniyuki Iwashima Dec. 14, 2023, 1:46 p.m. UTC | #4 
From: Shigeru Yoshida <syoshida@redhat.com>
Date: Thu, 14 Dec 2023 22:31:06 +0900 (JST)
> On Thu, 14 Dec 2023 17:46:22 +0900, Kuniyuki Iwashima wrote:
> > From: Shigeru Yoshida <syoshida@redhat.com>
> > Date: Thu, 14 Dec 2023 14:09:22 +0900
> >> The following NULL pointer dereference issue occurred:
> >> 
> >> BUG: kernel NULL pointer dereference, address: 0000000000000000
> >> <...>
> >> RIP: 0010:ccid_hc_tx_send_packet net/dccp/ccid.h:166 [inline]
> >> RIP: 0010:dccp_write_xmit+0x49/0x140 net/dccp/output.c:356
> >> <...>
> >> Call Trace:
> >>  <TASK>
> >>  dccp_sendmsg+0x642/0x7e0 net/dccp/proto.c:801
> >>  inet_sendmsg+0x63/0x90 net/ipv4/af_inet.c:846
> >>  sock_sendmsg_nosec net/socket.c:730 [inline]
> >>  __sock_sendmsg+0x83/0xe0 net/socket.c:745
> >>  ____sys_sendmsg+0x443/0x510 net/socket.c:2558
> >>  ___sys_sendmsg+0xe5/0x150 net/socket.c:2612
> >>  __sys_sendmsg+0xa6/0x120 net/socket.c:2641
> >>  __do_sys_sendmsg net/socket.c:2650 [inline]
> >>  __se_sys_sendmsg net/socket.c:2648 [inline]
> >>  __x64_sys_sendmsg+0x45/0x50 net/socket.c:2648
> >>  do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> >>  do_syscall_64+0x43/0x110 arch/x86/entry/common.c:82
> >>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> >> 
> >> sk_wait_event() returns an error (-EPIPE) if disconnect() is called on the
> >> socket waiting for the event. However, sk_stream_wait_connect() returns
> >> success, i.e. zero, even if sk_wait_event() returns -EPIPE, so a function
> >> that waits for a connection with sk_stream_wait_connect() may misbehave.
> >> 
> >> In the case of the above DCCP issue, dccp_sendmsg() is waiting for the
> >> connection. If disconnect() is called in concurrently, the above issue
> >> occurs.
> >> 
> >> This patch fixes the issue by returning error from sk_stream_wait_connect()
> >> if sk_wait_event() fails.
> >> 
> >> Fixes: 419ce133ab92 ("tcp: allow again tcp_disconnect() when threads are waiting")
> >> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > 
> > Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > 
> > I guess you picked this issue from syzbot's report.
> > https://lore.kernel.org/netdev/0000000000009e122006088a2b8d@google.com/
> > 
> > If so, let's give a proper credit to syzbot and its authors:
> > 
> > Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
> 
> Hi Kuniyuki-san,
> 
> Thank you very much for your review. I didn't notice the syzbot's
> report. Actually, I found this issue by running syzkaller on my
> machine.

Thanks for clarifying.

I'm also running syzkaller locally and used to add

  Reported-by: syzbot <syzkaller@googlegroups.com>

But, it was confusing for syzbot's owners, and I got a mail from one of
the authors, Aleksandr Nogikh.  Since then, if syzkaller found an issue
that was not on the syzbot dashboard, I have used

  Reported-by: syzkaller <syzkaller@googlegroups.com>

.  FWIW, here's Aleksandr's words from the mail.

---8<---
Maybe it would be just a little more clear if instead of
Reported-by: syzbot <syzkaller@googlegroups.com>
you'd write
Reported-by: syzkaller <syzkaller@googlegroups.com>
if the bug was found only by a local syzkaller instance, because
otherwise it implies that the bug was found by syzbot, which is not
really the case here :)
---8<---


> 
> Now, I tested this patch with syzbot, and it looks good.
> 
> Reported-and-tested-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com

This time, this tag is best.

Thanks!
Shigeru Yoshida Dec. 14, 2023, 1:58 p.m. UTC | #5 
On Thu, 14 Dec 2023 22:46:14 +0900, Kuniyuki Iwashima wrote:
> From: Shigeru Yoshida <syoshida@redhat.com>
> Date: Thu, 14 Dec 2023 22:31:06 +0900 (JST)
>> On Thu, 14 Dec 2023 17:46:22 +0900, Kuniyuki Iwashima wrote:
>> > From: Shigeru Yoshida <syoshida@redhat.com>
>> > Date: Thu, 14 Dec 2023 14:09:22 +0900
>> >> The following NULL pointer dereference issue occurred:
>> >> 
>> >> BUG: kernel NULL pointer dereference, address: 0000000000000000
>> >> <...>
>> >> RIP: 0010:ccid_hc_tx_send_packet net/dccp/ccid.h:166 [inline]
>> >> RIP: 0010:dccp_write_xmit+0x49/0x140 net/dccp/output.c:356
>> >> <...>
>> >> Call Trace:
>> >>  <TASK>
>> >>  dccp_sendmsg+0x642/0x7e0 net/dccp/proto.c:801
>> >>  inet_sendmsg+0x63/0x90 net/ipv4/af_inet.c:846
>> >>  sock_sendmsg_nosec net/socket.c:730 [inline]
>> >>  __sock_sendmsg+0x83/0xe0 net/socket.c:745
>> >>  ____sys_sendmsg+0x443/0x510 net/socket.c:2558
>> >>  ___sys_sendmsg+0xe5/0x150 net/socket.c:2612
>> >>  __sys_sendmsg+0xa6/0x120 net/socket.c:2641
>> >>  __do_sys_sendmsg net/socket.c:2650 [inline]
>> >>  __se_sys_sendmsg net/socket.c:2648 [inline]
>> >>  __x64_sys_sendmsg+0x45/0x50 net/socket.c:2648
>> >>  do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>> >>  do_syscall_64+0x43/0x110 arch/x86/entry/common.c:82
>> >>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
>> >> 
>> >> sk_wait_event() returns an error (-EPIPE) if disconnect() is called on the
>> >> socket waiting for the event. However, sk_stream_wait_connect() returns
>> >> success, i.e. zero, even if sk_wait_event() returns -EPIPE, so a function
>> >> that waits for a connection with sk_stream_wait_connect() may misbehave.
>> >> 
>> >> In the case of the above DCCP issue, dccp_sendmsg() is waiting for the
>> >> connection. If disconnect() is called in concurrently, the above issue
>> >> occurs.
>> >> 
>> >> This patch fixes the issue by returning error from sk_stream_wait_connect()
>> >> if sk_wait_event() fails.
>> >> 
>> >> Fixes: 419ce133ab92 ("tcp: allow again tcp_disconnect() when threads are waiting")
>> >> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>> > 
>> > Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
>> > 
>> > I guess you picked this issue from syzbot's report.
>> > https://lore.kernel.org/netdev/0000000000009e122006088a2b8d@google.com/
>> > 
>> > If so, let's give a proper credit to syzbot and its authors:
>> > 
>> > Reported-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
>> 
>> Hi Kuniyuki-san,
>> 
>> Thank you very much for your review. I didn't notice the syzbot's
>> report. Actually, I found this issue by running syzkaller on my
>> machine.
> 
> Thanks for clarifying.
> 
> I'm also running syzkaller locally and used to add
> 
>   Reported-by: syzbot <syzkaller@googlegroups.com>
> 
> But, it was confusing for syzbot's owners, and I got a mail from one of
> the authors, Aleksandr Nogikh.  Since then, if syzkaller found an issue
> that was not on the syzbot dashboard, I have used
> 
>   Reported-by: syzkaller <syzkaller@googlegroups.com>

Thanks for your information. This tag looks great, so I will use this
next time I send a fix found by local syzkaller :)

Thanks,
Shigeru

> 
> .  FWIW, here's Aleksandr's words from the mail.
> 
> ---8<---
> Maybe it would be just a little more clear if instead of
> Reported-by: syzbot <syzkaller@googlegroups.com>
> you'd write
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> if the bug was found only by a local syzkaller instance, because
> otherwise it implies that the bug was found by syzbot, which is not
> really the case here :)
> ---8<---
> 
> 
>> 
>> Now, I tested this patch with syzbot, and it looks good.
>> 
>> Reported-and-tested-by: syzbot+c71bc336c5061153b502@syzkaller.appspotmail.com
> 
> This time, this tag is best.
> 
> Thanks!
> 
>
patchwork-bot+netdevbpf@kernel.org Dec. 15, 2023, 10:50 a.m. UTC | #6 
Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 14 Dec 2023 14:09:22 +0900 you wrote:
> The following NULL pointer dereference issue occurred:
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> <...>
> RIP: 0010:ccid_hc_tx_send_packet net/dccp/ccid.h:166 [inline]
> RIP: 0010:dccp_write_xmit+0x49/0x140 net/dccp/output.c:356
> <...>
> Call Trace:
>  <TASK>
>  dccp_sendmsg+0x642/0x7e0 net/dccp/proto.c:801
>  inet_sendmsg+0x63/0x90 net/ipv4/af_inet.c:846
>  sock_sendmsg_nosec net/socket.c:730 [inline]
>  __sock_sendmsg+0x83/0xe0 net/socket.c:745
>  ____sys_sendmsg+0x443/0x510 net/socket.c:2558
>  ___sys_sendmsg+0xe5/0x150 net/socket.c:2612
>  __sys_sendmsg+0xa6/0x120 net/socket.c:2641
>  __do_sys_sendmsg net/socket.c:2650 [inline]
>  __se_sys_sendmsg net/socket.c:2648 [inline]
>  __x64_sys_sendmsg+0x45/0x50 net/socket.c:2648
>  do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>  do_syscall_64+0x43/0x110 arch/x86/entry/common.c:82
>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> [...]

Here is the summary with links:
  - [net] net: Return error from sk_stream_wait_connect() if sk_wait_event() fails
    https://git.kernel.org/netdev/net/c/cac23b7d7627

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/core/stream.c b/net/core/stream.c
index 96fbcb9bbb30..b16dfa568a2d 100644
--- a/net/core/stream.c
+++ b/net/core/stream.c
@@ -79,7 +79,7 @@  int sk_stream_wait_connect(struct sock *sk, long *timeo_p)
 		remove_wait_queue(sk_sleep(sk), &wait);
 		sk->sk_write_pending--;
 	} while (!done);
-	return 0;
+	return done < 0 ? done : 0;
 }
 EXPORT_SYMBOL(sk_stream_wait_connect);