[PATCH FINAL] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

[PATCH FINAL] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

[Jacek Luczak]

During the sctp_close() call, we do not use rcu primitives to
destroy the address list attached to the endpoint.  At the same
time, we do the removal of addresses from this list before
attempting to remove the socket from the port hash

As a result, it is possible for another process to find the socket
in the port hash that is in the process of being closed.  It then
proceeds to traverse the address list to find the conflict, only
to have that address list suddenly disappear without rcu() critical
section.

Fix issue by closing address list removal inside RCU critical
section.

Race can result in a kernel crash with general protection fault or
kernel NULL pointer dereference:

kernel: general protection fault: 0000 [#1] SMP
kernel: RIP: 0010:[<ffffffffa02f3dde>]  [<ffffffffa02f3dde>] sctp_bind_addr_conflict+0x64/0x82 [sctp]
kernel: Call Trace:
kernel:  [<ffffffffa02f415f>] ? sctp_get_port_local+0x17b/0x2a3 [sctp]
kernel:  [<ffffffffa02f3d45>] ? sctp_bind_addr_match+0x33/0x68 [sctp]
kernel:  [<ffffffffa02f4416>] ? sctp_do_bind+0xd3/0x141 [sctp]
kernel:  [<ffffffffa02f5030>] ? sctp_bindx_add+0x4d/0x8e [sctp]
kernel:  [<ffffffffa02f5183>] ? sctp_setsockopt_bindx+0x112/0x4a4 [sctp]
kernel:  [<ffffffff81089e82>] ? generic_file_aio_write+0x7f/0x9b
kernel:  [<ffffffffa02f763e>] ? sctp_setsockopt+0x14f/0xfee [sctp]
kernel:  [<ffffffff810c11fb>] ? do_sync_write+0xab/0xeb
kernel:  [<ffffffff810e82ab>] ? fsnotify+0x239/0x282
kernel:  [<ffffffff810c2462>] ? alloc_file+0x18/0xb1
kernel:  [<ffffffff8134a0b1>] ? compat_sys_setsockopt+0x1a5/0x1d9
kernel:  [<ffffffff8134aaf1>] ? compat_sys_socketcall+0x143/0x1a4
kernel:  [<ffffffff810467dc>] ? sysenter_dispatch+0x7/0x32

Signed-off-by: Jacek Luczak <luczak.jacek@gmail.com>
Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>

---
 net/sctp/bind_addr.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index faf71d1..6150ac5 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -140,14 +140,12 @@ void sctp_bind_addr_init(struct sctp_bind_addr *bp, __u16 port)
 /* Dispose of the address list. */
 static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
 {
-	struct sctp_sockaddr_entry *addr;
-	struct list_head *pos, *temp;
+	struct sctp_sockaddr_entry *addr, *temp;
 
 	/* Empty the bind address list. */
-	list_for_each_safe(pos, temp, &bp->address_list) {
-		addr = list_entry(pos, struct sctp_sockaddr_entry, list);
-		list_del(pos);
-		kfree(addr);
+	list_for_each_entry_safe(addr, temp, &bp->address_list, list) {
+		list_del_rcu(&addr->list);
+		call_rcu(&addr->rcu, sctp_local_addr_free);
 		SCTP_DBG_OBJCNT_DEC(addr);
 	}
 }

Re: [PATCH FINAL] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

[Eric Dumazet]

Le jeudi 19 mai 2011 à 21:55 +0200, Jacek Luczak a écrit :
> During the sctp_close() call, we do not use rcu primitives to
> destroy the address list attached to the endpoint.  At the same
> time, we do the removal of addresses from this list before
> attempting to remove the socket from the port hash
> 
> As a result, it is possible for another process to find the socket
> in the port hash that is in the process of being closed.  It then
> proceeds to traverse the address list to find the conflict, only
> to have that address list suddenly disappear without rcu() critical
> section.
> 
> Fix issue by closing address list removal inside RCU critical
> section.
> 
> Race can result in a kernel crash with general protection fault or
> kernel NULL pointer dereference:
> 

> Signed-off-by: Jacek Luczak <luczak.jacek@gmail.com>
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> 
> ---
>  net/sctp/bind_addr.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH FINAL] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 May 2011 21:58:21 +0200

> Le jeudi 19 mai 2011 à 21:55 +0200, Jacek Luczak a écrit :
>> During the sctp_close() call, we do not use rcu primitives to
>> destroy the address list attached to the endpoint.  At the same
>> time, we do the removal of addresses from this list before
>> attempting to remove the socket from the port hash
>> 
>> As a result, it is possible for another process to find the socket
>> in the port hash that is in the process of being closed.  It then
>> proceeds to traverse the address list to find the conflict, only
>> to have that address list suddenly disappear without rcu() critical
>> section.
>> 
>> Fix issue by closing address list removal inside RCU critical
>> section.
>> 
>> Race can result in a kernel crash with general protection fault or
>> kernel NULL pointer dereference:
>> 
> 
>> Signed-off-by: Jacek Luczak <luczak.jacek@gmail.com>
>> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>> CC: Eric Dumazet <eric.dumazet@gmail.com>
>> 
>> ---
>>  net/sctp/bind_addr.c |   10 ++++------
>>  1 files changed, 4 insertions(+), 6 deletions(-)
>> 
> 
> Reviewed-by: Eric Dumazet <eric.dumazet@gmail.com>
> 

Applied and queued up for -stable.

Re: [PATCH FINAL] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

[Dave Jones]

On Thu, May 19, 2011 at 09:55:13PM +0200, Jacek Luczak wrote:
 > During the sctp_close() call, we do not use rcu primitives to
 > destroy the address list attached to the endpoint.  At the same
 > time, we do the removal of addresses from this list before
 > attempting to remove the socket from the port hash
 > 
 > As a result, it is possible for another process to find the socket
 > in the port hash that is in the process of being closed.  It then
 > proceeds to traverse the address list to find the conflict, only
 > to have that address list suddenly disappear without rcu() critical
 > section.
 > 
 > Fix issue by closing address list removal inside RCU critical
 > section.
 > 
 > Race can result in a kernel crash with general protection fault or
 > kernel NULL pointer dereference:
 > 
 > kernel: general protection fault: 0000 [#1] SMP
 > kernel: RIP: 0010:[<ffffffffa02f3dde>]  [<ffffffffa02f3dde>] sctp_bind_addr_conflict+0x64/0x82 [sctp]
 > kernel: Call Trace:
 > kernel:  [<ffffffffa02f415f>] ? sctp_get_port_local+0x17b/0x2a3 [sctp]
 > kernel:  [<ffffffffa02f3d45>] ? sctp_bind_addr_match+0x33/0x68 [sctp]
 > kernel:  [<ffffffffa02f4416>] ? sctp_do_bind+0xd3/0x141 [sctp]
 > kernel:  [<ffffffffa02f5030>] ? sctp_bindx_add+0x4d/0x8e [sctp]
 > kernel:  [<ffffffffa02f5183>] ? sctp_setsockopt_bindx+0x112/0x4a4 [sctp]
 > kernel:  [<ffffffff81089e82>] ? generic_file_aio_write+0x7f/0x9b
 > kernel:  [<ffffffffa02f763e>] ? sctp_setsockopt+0x14f/0xfee [sctp]
 > kernel:  [<ffffffff810c11fb>] ? do_sync_write+0xab/0xeb
 > kernel:  [<ffffffff810e82ab>] ? fsnotify+0x239/0x282
 > kernel:  [<ffffffff810c2462>] ? alloc_file+0x18/0xb1
 > kernel:  [<ffffffff8134a0b1>] ? compat_sys_setsockopt+0x1a5/0x1d9
 > kernel:  [<ffffffff8134aaf1>] ? compat_sys_socketcall+0x143/0x1a4
 > kernel:  [<ffffffff810467dc>] ? sysenter_dispatch+0x7/0x32
 > 
 > Signed-off-by: Jacek Luczak <luczak.jacek@gmail.com>
 > Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
 > CC: Eric Dumazet <eric.dumazet@gmail.com>
 > 
 > ---
 >  net/sctp/bind_addr.c |   10 ++++------
 >  1 files changed, 4 insertions(+), 6 deletions(-)
 > 
 > diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
 > index faf71d1..6150ac5 100644
 > --- a/net/sctp/bind_addr.c
 > +++ b/net/sctp/bind_addr.c
 > @@ -140,14 +140,12 @@ void sctp_bind_addr_init(struct sctp_bind_addr *bp, __u16 port)
 >  /* Dispose of the address list. */
 >  static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
 >  {
 > -	struct sctp_sockaddr_entry *addr;
 > -	struct list_head *pos, *temp;
 > +	struct sctp_sockaddr_entry *addr, *temp;
 >  
 >  	/* Empty the bind address list. */
 > -	list_for_each_safe(pos, temp, &bp->address_list) {
 > -		addr = list_entry(pos, struct sctp_sockaddr_entry, list);
 > -		list_del(pos);
 > -		kfree(addr);
 > +	list_for_each_entry_safe(addr, temp, &bp->address_list, list) {
 > +		list_del_rcu(&addr->list);
 > +		call_rcu(&addr->rcu, sctp_local_addr_free);
 >  		SCTP_DBG_OBJCNT_DEC(addr);

Just saw this land in Linus tree, and it broke the build for me..

net/sctp/bind_addr.c: In function ‘sctp_bind_addr_clean’:
net/sctp/bind_addr.c:148:24: error: ‘sctp_local_addr_free’ undeclared (first use in this function)
net/sctp/bind_addr.c:148:24: note: each undeclared identifier is reported only once for each function it appears in
make[2]: *** [net/sctp/bind_addr.o] Error 1

	Dave

 

Re: [PATCH FINAL] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

[David Miller]

From: Dave Jones <davej@redhat.com>
Date: Fri, 20 May 2011 19:27:17 -0400

> Just saw this land in Linus tree, and it broke the build for me..
> 
> net/sctp/bind_addr.c: In function ‘sctp_bind_addr_clean’:
> net/sctp/bind_addr.c:148:24: error: ‘sctp_local_addr_free’ undeclared (first use in this function)
> net/sctp/bind_addr.c:148:24: note: each undeclared identifier is reported only once for each function it appears in
> make[2]: *** [net/sctp/bind_addr.o] Error 1

Yes this interacted and merged badly with the kfree_rcu() changes,
I'll fix this up, thanks.