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

[Shigeru Yoshida]

On Thu, 14 Dec 2023 17:46:22 +0900, Kuniyuki Iwashima wrote:
> From: Shigeru Yoshida <syoshida@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> 
> Reviewed-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> 
> I guess you picked this issue from syzbot's report.
> https://lore.kernel.org/netdev/0000000000009e122006088a2b8d@xxxxxxxxxx/
> 
> If so, let's give a proper credit to syzbot and its authors:
> 
> Reported-by: syzbot+c71bc336c5061153b502@xxxxxxxxxxxxxxxxxxxxxxxxx

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@xxxxxxxxxxxxxxxxxxxxxxxxx

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
>> 
>