Skip to contentSkip to conversation

security/selinux/netlabel.c: fix double free

PATCH In review Adrian Bunk 2 participants 4 messages Jan 28, 2008

Subsystems (3)
Networking Network drivers SELinux security module
lore
Share & tools
lore

Adrian Bunk

author
#0 lore raw

PATCH 1/1 security/selinux/netlabel.c: fix double free

This patch fixes a double free (security_netlbl_sid_to_secattr() already calls netlbl_secattr_destroy() when it returns !0) introduced by commit 45c950e0f839fded922ebc0bfd59b1081cc71b70 and spotted by the Coverity checker.

Signed-off-by Adrian Bunk <[email protected]>

---

security/selinux/netlabel.c renamed from security/selinux/netlabel.c.old +1 -1
@@ -58,22 +58,22 @@ static int selinux_netlbl_sock_setsid(st
 	rc = security_netlbl_sid_to_secattr(sid, &secattr);
 	if (rc != 0)
 		goto sock_setsid_return;
 	rc = netlbl_sock_setattr(sk, &secattr);
 	if (rc == 0) {
 		spin_lock_bh(&sksec->nlbl_lock);
 		sksec->nlbl_state = NLBL_LABELED;
 		spin_unlock_bh(&sksec->nlbl_lock);
 	}
 
-sock_setsid_return:
 	netlbl_secattr_destroy(&secattr);
+sock_setsid_return:
 	return rc;
 }
 
 /**
  * selinux_netlbl_cache_invalidate - Invalidate the NetLabel cache
  *
  * Description:
  * Invalidate the NetLabel security attribute mapping cache.
  *
  */
#1 lore raw
Quoted 4 lines · Adrian Bunk

This patch fixes a double free (security_netlbl_sid_to_secattr() already calls netlbl_secattr_destroy() when it returns !0) introduced by commit 45c950e0f839fded922ebc0bfd59b1081cc71b70 and spotted by the Coverity checker.

Hi Adrian,

Thanks for finding this mistake, however, I'd rather see it fixed by removing the netlbl_secattr_destroy() call in security_netlbl_sid_to_secattr() as it really shouldn't be there anymore. We moved the matching _init() call into selinux_netlbl_sock_setsid() and I'd like to see the _init() and _destroy() calls done in the same function. I can push a revised patch for this if you would prefer, otherwise I'll be happy to ack an updated version ...

Quoted 30 lines
Signed-off-by Adrian Bunk <[email protected]>

---
--- linux-2.6/security/selinux/netlabel.c.old 2008-01-23
00:38:19.000000000 +0200 +++
linux-2.6/security/selinux/netlabel.c 2008-01-23 00:39:09.000000000
+0200 @@ -58,22 +58,22 @@ static int selinux_netlbl_sock_setsid(st rc = security_netlbl_sid_to_secattr(sid, &secattr);

security/selinux/netlabel.c renamed from security/selinux/netlabel.c.old +1 -1
 	if (rc != 0)
 		goto sock_setsid_return;
 	rc = netlbl_sock_setattr(sk, &secattr);
 	if (rc == 0) {
 		spin_lock_bh(&sksec->nlbl_lock);
 		sksec->nlbl_state = NLBL_LABELED;
 		spin_unlock_bh(&sksec->nlbl_lock);
 	}
 
-sock_setsid_return:
 	netlbl_secattr_destroy(&secattr);
+sock_setsid_return:
 	return rc;
 }
 
 /**
  * selinux_netlbl_cache_invalidate - Invalidate the NetLabel cache
  *
  * Description:
  * Invalidate the NetLabel security attribute mapping cache.
  *
  */
Signature · 2 lines
paul moore
linux security @ hp
author
#2 lore raw
Quoted 7 lines · Paul Moore
Quoted 4 lines · Adrian Bunk

This patch fixes a double free (security_netlbl_sid_to_secattr() already calls netlbl_secattr_destroy() when it returns !0) introduced by commit 45c950e0f839fded922ebc0bfd59b1081cc71b70 and spotted by the Coverity checker.

Hi Adrian,

Hi Paul,

Quoted 8 lines

Thanks for finding this mistake, however, I'd rather see it fixed by removing the netlbl_secattr_destroy() call in security_netlbl_sid_to_secattr() as it really shouldn't be there anymore. We moved the matching _init() call into selinux_netlbl_sock_setsid() and I'd like to see the _init() and _destroy() calls done in the same function. I can push a revised patch for this if you would prefer, otherwise I'll be happy to ack an updated version ...

doing the patch is trivial but you are able to write a better changelog for it - just push a revised patch.

Quoted 1 line

paul moore

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#3 lore raw
Quoted 10 lines · Adrian Bunk
Quoted 8 lines · Paul Moore

Thanks for finding this mistake, however, I'd rather see it fixed by removing the netlbl_secattr_destroy() call in security_netlbl_sid_to_secattr() as it really shouldn't be there anymore. We moved the matching _init() call into selinux_netlbl_sock_setsid() and I'd like to see the _init() and _destroy() calls done in the same function. I can push a revised patch for this if you would prefer, otherwise I'll be happy to ack an updated version ...

Quoted 2 lines

doing the patch is trivial but you are able to write a better changelog for it - just push a revised patch.

Will do, thanks.

Signature · 2 lines
paul moore
linux security @ hp