diff mbox series

net: tipc: fix general protection fault in tipc_conn_delete_sub

Message ID 20200727131057.7a3of3hhsld4ng5t@pesu.pes.edu
State Changes Requested
Delegated to: David Miller
Headers show
Series net: tipc: fix general protection fault in tipc_conn_delete_sub | expand

Commit Message

B K Karthik July 27, 2020, 1:10 p.m. UTC
fix a general protection fault in tipc_conn_delete_sub
by checking for the existance of con->server.
prevent a null-ptr-deref by returning -EINVAL when
con->server is NULL

general protection fault, probably for non-canonical address 0xdffffc0000000014: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x00000000000000a0-0x00000000000000a7]
CPU: 1 PID: 113 Comm: kworker/u4:3 Not tainted 5.6.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: tipc_send tipc_conn_send_work
RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000076c000 CR3: 000000009441d000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 tipc_conn_send_to_sock+0x380/0x560 net/tipc/topsrv.c:266
 tipc_conn_send_work+0x6f/0x90 net/tipc/topsrv.c:304
 process_one_work+0x965/0x16a0 kernel/workqueue.c:2266
 worker_thread+0x96/0xe20 kernel/workqueue.c:2412
 kthread+0x388/0x470 kernel/kthread.c:268
 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Modules linked in:
---[ end trace 2c161a84be832606 ]---
RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020800000 CR3: 0000000091b8e000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

Reported-and-tested-by: syzbot+55a38037455d0351efd3@syzkaller.appspotmail.com
Signed-off-by: B K Karthik <bkkarthik@pesu.pes.edu>
---
 net/tipc/topsrv.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Greg KH July 27, 2020, 1:22 p.m. UTC | #1
On Mon, Jul 27, 2020 at 06:40:57PM +0530, B K Karthik wrote:
> fix a general protection fault in tipc_conn_delete_sub
> by checking for the existance of con->server.
> prevent a null-ptr-deref by returning -EINVAL when
> con->server is NULL
> 
> general protection fault, probably for non-canonical address 0xdffffc0000000014: 0000 [#1] PREEMPT SMP KASAN
> KASAN: null-ptr-deref in range [0x00000000000000a0-0x00000000000000a7]
> CPU: 1 PID: 113 Comm: kworker/u4:3 Not tainted 5.6.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: tipc_send tipc_conn_send_work
> RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
> Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
> RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
> RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
> RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
> R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
> R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
> FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000000076c000 CR3: 000000009441d000 CR4: 00000000001406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  tipc_conn_send_to_sock+0x380/0x560 net/tipc/topsrv.c:266
>  tipc_conn_send_work+0x6f/0x90 net/tipc/topsrv.c:304
>  process_one_work+0x965/0x16a0 kernel/workqueue.c:2266
>  worker_thread+0x96/0xe20 kernel/workqueue.c:2412
>  kthread+0x388/0x470 kernel/kthread.c:268
>  ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> Modules linked in:
> ---[ end trace 2c161a84be832606 ]---
> RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
> Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
> RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
> RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
> RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
> R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
> R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
> FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020800000 CR3: 0000000091b8e000 CR4: 00000000001406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> 
> Reported-and-tested-by: syzbot+55a38037455d0351efd3@syzkaller.appspotmail.com
> Signed-off-by: B K Karthik <bkkarthik@pesu.pes.edu>
> ---
>  net/tipc/topsrv.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 1489cfb941d8..6c8d0c6bb112 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -255,6 +255,9 @@ static void tipc_conn_send_to_sock(struct tipc_conn *con)
>  	int count = 0;
>  	int ret;
>  
> +	if (!con->server)
> +		return -EINVAL;

What is wrong with looking at the srv local variable instead?

And how is server getting set to NULL and this function still being
called?

thanks,

greg k-h
B K Karthik July 27, 2020, 2:16 p.m. UTC | #2
On Mon, Jul 27, 2020 at 6:53 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 27, 2020 at 06:40:57PM +0530, B K Karthik wrote:
> > fix a general protection fault in tipc_conn_delete_sub
> > by checking for the existance of con->server.
> > prevent a null-ptr-deref by returning -EINVAL when
> > con->server is NULL
> >
> > general protection fault, probably for non-canonical address 0xdffffc0000000014: 0000 [#1] PREEMPT SMP KASAN
> > KASAN: null-ptr-deref in range [0x00000000000000a0-0x00000000000000a7]
> > CPU: 1 PID: 113 Comm: kworker/u4:3 Not tainted 5.6.0-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > Workqueue: tipc_send tipc_conn_send_work
> > RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
> > Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
> > RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
> > RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
> > RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
> > RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
> > R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
> > R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
> > FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 000000000076c000 CR3: 000000009441d000 CR4: 00000000001406e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> >  tipc_conn_send_to_sock+0x380/0x560 net/tipc/topsrv.c:266
> >  tipc_conn_send_work+0x6f/0x90 net/tipc/topsrv.c:304
> >  process_one_work+0x965/0x16a0 kernel/workqueue.c:2266
> >  worker_thread+0x96/0xe20 kernel/workqueue.c:2412
> >  kthread+0x388/0x470 kernel/kthread.c:268
> >  ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> > Modules linked in:
> > ---[ end trace 2c161a84be832606 ]---
> > RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
> > Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
> > RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
> > RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
> > RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
> > RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
> > R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
> > R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
> > FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000020800000 CR3: 0000000091b8e000 CR4: 00000000001406e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >
> > Reported-and-tested-by: syzbot+55a38037455d0351efd3@syzkaller.appspotmail.com
> > Signed-off-by: B K Karthik <bkkarthik@pesu.pes.edu>
> > ---
> >  net/tipc/topsrv.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> > index 1489cfb941d8..6c8d0c6bb112 100644
> > --- a/net/tipc/topsrv.c
> > +++ b/net/tipc/topsrv.c
> > @@ -255,6 +255,9 @@ static void tipc_conn_send_to_sock(struct tipc_conn *con)
> >       int count = 0;
> >       int ret;
> >
> > +     if (!con->server)
> > +             return -EINVAL;
>
> What is wrong with looking at the srv local variable instead?
>
> And how is server getting set to NULL and this function still being
> called?

tipc_conn_send_work makes a call to connected() which just returns con
&& test_bit(CF_CONNECTED, &con->flags)
maybe we can add this check to the implementation of connection() if
you agree, but I found this solution to be fairly simpler because I'm
not sure where else connected() is being used, and I did not want to
introduce redundant function calls.

Yes we can replace con->server with the local variable srv. Extremely
sorry, I hadn't noticed it earlier.

please let me know if i've wrongly understood any of these.
thanks,

karthik
Greg KH July 27, 2020, 2:24 p.m. UTC | #3
On Mon, Jul 27, 2020 at 07:46:05PM +0530, B K Karthik wrote:
> On Mon, Jul 27, 2020 at 6:53 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jul 27, 2020 at 06:40:57PM +0530, B K Karthik wrote:
> > > fix a general protection fault in tipc_conn_delete_sub
> > > by checking for the existance of con->server.
> > > prevent a null-ptr-deref by returning -EINVAL when
> > > con->server is NULL
> > >
> > > general protection fault, probably for non-canonical address 0xdffffc0000000014: 0000 [#1] PREEMPT SMP KASAN
> > > KASAN: null-ptr-deref in range [0x00000000000000a0-0x00000000000000a7]
> > > CPU: 1 PID: 113 Comm: kworker/u4:3 Not tainted 5.6.0-syzkaller #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > Workqueue: tipc_send tipc_conn_send_work
> > > RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
> > > Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
> > > RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
> > > RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
> > > RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
> > > RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
> > > R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
> > > R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
> > > FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 000000000076c000 CR3: 000000009441d000 CR4: 00000000001406e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > Call Trace:
> > >  tipc_conn_send_to_sock+0x380/0x560 net/tipc/topsrv.c:266
> > >  tipc_conn_send_work+0x6f/0x90 net/tipc/topsrv.c:304
> > >  process_one_work+0x965/0x16a0 kernel/workqueue.c:2266
> > >  worker_thread+0x96/0xe20 kernel/workqueue.c:2412
> > >  kthread+0x388/0x470 kernel/kthread.c:268
> > >  ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> > > Modules linked in:
> > > ---[ end trace 2c161a84be832606 ]---
> > > RIP: 0010:tipc_conn_delete_sub+0x54/0x440 net/tipc/topsrv.c:231
> > > Code: 48 c1 ea 03 80 3c 02 00 0f 85 f0 03 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 18 48 8d bd a0 00 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 03 00 00 48 c7 c0 34 0b 8a 8a 4c 8b a5 a0 00
> > > RSP: 0018:ffffc900012d7b58 EFLAGS: 00010206
> > > RAX: dffffc0000000000 RBX: ffff8880a8269c00 RCX: ffffffff8789ca01
> > > RDX: 0000000000000014 RSI: ffffffff8789a059 RDI: 00000000000000a0
> > > RBP: 0000000000000000 R08: ffff8880a8d88380 R09: fffffbfff18577a8
> > > R10: fffffbfff18577a7 R11: ffffffff8c2bbd3f R12: dffffc0000000000
> > > R13: ffff888093d35a18 R14: ffff8880a8269c00 R15: ffff888093d35a00
> > > FS:  0000000000000000(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 0000000020800000 CR3: 0000000091b8e000 CR4: 00000000001406e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > >
> > > Reported-and-tested-by: syzbot+55a38037455d0351efd3@syzkaller.appspotmail.com
> > > Signed-off-by: B K Karthik <bkkarthik@pesu.pes.edu>
> > > ---
> > >  net/tipc/topsrv.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> > > index 1489cfb941d8..6c8d0c6bb112 100644
> > > --- a/net/tipc/topsrv.c
> > > +++ b/net/tipc/topsrv.c
> > > @@ -255,6 +255,9 @@ static void tipc_conn_send_to_sock(struct tipc_conn *con)
> > >       int count = 0;
> > >       int ret;
> > >
> > > +     if (!con->server)
> > > +             return -EINVAL;
> >
> > What is wrong with looking at the srv local variable instead?
> >
> > And how is server getting set to NULL and this function still being
> > called?
> 
> tipc_conn_send_work makes a call to connected() which just returns con
> && test_bit(CF_CONNECTED, &con->flags)
> maybe we can add this check to the implementation of connection() if
> you agree, but I found this solution to be fairly simpler because I'm
> not sure where else connected() is being used, and I did not want to
> introduce redundant function calls.

That's not what I asked here at all.

greg k-h
kernel test robot July 27, 2020, 3:52 p.m. UTC | #4
Hi K,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.8-rc7 next-20200724]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/B-K-Karthik/net-tipc-fix-general-protection-fault-in-tipc_conn_delete_sub/20200727-211330
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 92ed301919932f777713b9172e525674157e983d
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/tipc/topsrv.c: In function 'tipc_conn_send_to_sock':
>> net/tipc/topsrv.c:259:10: warning: 'return' with a value, in function returning void [-Wreturn-type]
     259 |   return -EINVAL;
         |          ^
   net/tipc/topsrv.c:247:13: note: declared here
     247 | static void tipc_conn_send_to_sock(struct tipc_conn *con)
         |             ^~~~~~~~~~~~~~~~~~~~~~

vim +/return +259 net/tipc/topsrv.c

   246	
   247	static void tipc_conn_send_to_sock(struct tipc_conn *con)
   248	{
   249		struct list_head *queue = &con->outqueue;
   250		struct tipc_topsrv *srv = con->server;
   251		struct outqueue_entry *e;
   252		struct tipc_event *evt;
   253		struct msghdr msg;
   254		struct kvec iov;
   255		int count = 0;
   256		int ret;
   257	
   258		if (!con->server)
 > 259			return -EINVAL;
   260	
   261		spin_lock_bh(&con->outqueue_lock);
   262	
   263		while (!list_empty(queue)) {
   264			e = list_first_entry(queue, struct outqueue_entry, list);
   265			evt = &e->evt;
   266			spin_unlock_bh(&con->outqueue_lock);
   267	
   268			if (e->inactive)
   269				tipc_conn_delete_sub(con, &evt->s);
   270	
   271			memset(&msg, 0, sizeof(msg));
   272			msg.msg_flags = MSG_DONTWAIT;
   273			iov.iov_base = evt;
   274			iov.iov_len = sizeof(*evt);
   275			msg.msg_name = NULL;
   276	
   277			if (con->sock) {
   278				ret = kernel_sendmsg(con->sock, &msg, &iov,
   279						     1, sizeof(*evt));
   280				if (ret == -EWOULDBLOCK || ret == 0) {
   281					cond_resched();
   282					return;
   283				} else if (ret < 0) {
   284					return tipc_conn_close(con);
   285				}
   286			} else {
   287				tipc_topsrv_kern_evt(srv->net, evt);
   288			}
   289	
   290			/* Don't starve users filling buffers */
   291			if (++count >= MAX_SEND_MSG_COUNT) {
   292				cond_resched();
   293				count = 0;
   294			}
   295			spin_lock_bh(&con->outqueue_lock);
   296			list_del(&e->list);
   297			kfree(e);
   298		}
   299		spin_unlock_bh(&con->outqueue_lock);
   300	}
   301	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
David Miller July 27, 2020, 7:38 p.m. UTC | #5
From: kernel test robot <lkp@intel.com>
Date: Mon, 27 Jul 2020 23:52:50 +0800

> All warnings (new ones prefixed by >>):
> 
>    net/tipc/topsrv.c: In function 'tipc_conn_send_to_sock':
>>> net/tipc/topsrv.c:259:10: warning: 'return' with a value, in function returning void [-Wreturn-type]
>      259 |   return -EINVAL;
>          |          ^
>    net/tipc/topsrv.c:247:13: note: declared here
>      247 | static void tipc_conn_send_to_sock(struct tipc_conn *con)
>          |             ^~~~~~~~~~~~~~~~~~~~~~

Please look at the compiler output when you submit changes.
Ying Xue July 28, 2020, 4:15 p.m. UTC | #6
On 7/27/20 10:24 PM, Greg KH wrote:
>>>> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
>>>> index 1489cfb941d8..6c8d0c6bb112 100644
>>>> --- a/net/tipc/topsrv.c
>>>> +++ b/net/tipc/topsrv.c
>>>> @@ -255,6 +255,9 @@ static void tipc_conn_send_to_sock(struct tipc_conn *con)
>>>>       int count = 0;
>>>>       int ret;
>>>>
>>>> +     if (!con->server)
>>>> +             return -EINVAL;
>>> What is wrong with looking at the srv local variable instead?
>>>
>>> And how is server getting set to NULL and this function still being
>>> called?
>> tipc_conn_send_work makes a call to connected() which just returns con
>> && test_bit(CF_CONNECTED, &con->flags)
>> maybe we can add this check to the implementation of connection() if
>> you agree, but I found this solution to be fairly simpler because I'm
>> not sure where else connected() is being used, and I did not want to
>> introduce redundant function calls.
> That's not what I asked here at all
I agreed with Greg. The key problem is that we need to understand why
con->server got NULL, otherwise, we probably just hide the issue by
checking if it's NULL.

The topology server is created in kernel space as an internal TIPC
server and its life cycle is the same as TIPC network namespace.
Whenever the topology server accepts a connection from its client, it
will create a "con" which will be used to talk to the client and the
topology server instance (ie, "topsrv") will be attached to
"con->server". In theory, "con" cannot be died before its server, as a
result, con->server cannot become NULL in tipc_conn_send_to_sock().

So I suspect there is other potential issues which caused this problem,
for example, the refcount of "con" is not properly taken or put and this
case is triggered before of use-after-free, or race condition etc.
diff mbox series

Patch

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 1489cfb941d8..6c8d0c6bb112 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -255,6 +255,9 @@  static void tipc_conn_send_to_sock(struct tipc_conn *con)
 	int count = 0;
 	int ret;
 
+	if (!con->server)
+		return -EINVAL;
+
 	spin_lock_bh(&con->outqueue_lock);
 
 	while (!list_empty(queue)) {