SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

PATCH In review Jacek Luczak 4 participants 5 messages May 19, 2011 – May 21, 2011

Subsystems (3)

Networking Network drivers SCTP protocol

lore

Share & tools

lore

Jacek Luczak

author

2011-05-19 19:55 UTC #0 lore raw

PATCH 1/1 SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()

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 <[email protected]>

Acked-by Vlad Yasevich <[email protected]>

CC Eric Dumazet <[email protected]>

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

net/sctp/bind_addr.c +4 -6

index faf71d1..6150ac5 100644
@@ -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);
 	}
 }

Eric Dumazet

Reviewed-by on v1 1/1

2011-05-19 19:58 UTC #1 lore raw

Quoted 18 lines · 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:

Quoted 8 lines

Signed-off-by Jacek Luczak <[email protected]>
Acked-by Vlad Yasevich <[email protected]>
CC Eric Dumazet <[email protected]>
---
 net/sctp/bind_addr.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

Reviewed-by Eric Dumazet <[email protected]>

David Miller

maintainer

2011-05-19 20:19 UTC #2 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 19 May 2011 21:58:21 +0200

Quoted 23 lines · Jacek Luczak

Quoted 22 lines
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 <[email protected]>
Acked-by Vlad Yasevich <[email protected]>
CC Eric Dumazet <[email protected]>

Quoted 7 lines

Quoted 4 lines
---
 net/sctp/bind_addr.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
Reviewed-by Eric Dumazet <[email protected]>

Applied and queued up for -stable.

Dave Jones

2011-05-20 23:27 UTC #3 lore raw

In reply to Jacek Luczak

Quoted 53 lines · 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 <[email protected]>
Acked-by Vlad Yasevich <[email protected]>
CC Eric Dumazet <[email protected]>
---
 net/sctp/bind_addr.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
net/sctp/bind_addr.c +1 -2
index faf71d1..6150ac5 100644
@@ -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;

net/sctp/bind_addr.c void sctp_bind_addr_init(struct sctp_bind_addr *bp, __u16 port) · line 149

Quoted 10 lines

net/sctp/bind_addr.c +3 -4

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

David Miller

maintainer

2011-05-21 06:06 UTC #4 lore raw

From: Dave Jones <[email protected]>
Date: Fri, 20 May 2011 19:27:17 -0400

Quoted 6 lines

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.