Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common
From: Dongliang Mu <hidden>
Date: 2021-06-10 12:16:49
Also in:
linux-security-module, lkml
Subsystem:
networking [general], networking [labeled] (netlabel, labeled ipsec, secmark), the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Paul Moore, Linus Torvalds
On Thu, Jun 10, 2021 at 6:45 PM Liu Shixin [off-list ref] wrote:
On 2021/6/10 11:08, Dongliang Mu wrote:quoted
On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin [off-list ref] wrote:quoted
Hulk Robot reported memory leak in netlbl_mgmt_add_common. The problem is non-freed map in case of netlbl_domhsh_add() failed. BUG: memory leak unreferenced object 0xffff888100ab7080 (size 96): comm "syz-executor537", pid 360, jiffies 4294862456 (age 22.678s) hex dump (first 32 bytes): 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ fe 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 ................ backtrace: [<0000000008b40026>] netlbl_mgmt_add_common.isra.0+0xb2a/0x1b40 [<000000003be10950>] netlbl_mgmt_add+0x271/0x3c0 [<00000000c70487ed>] genl_family_rcv_msg_doit.isra.0+0x20e/0x320 [<000000001f2ff614>] genl_rcv_msg+0x2bf/0x4f0 [<0000000089045792>] netlink_rcv_skb+0x134/0x3d0 [<0000000020e96fdd>] genl_rcv+0x24/0x40 [<0000000042810c66>] netlink_unicast+0x4a0/0x6a0 [<000000002e1659f0>] netlink_sendmsg+0x789/0xc70 [<000000006e43415f>] sock_sendmsg+0x139/0x170 [<00000000680a73d7>] ____sys_sendmsg+0x658/0x7d0 [<0000000065cbb8af>] ___sys_sendmsg+0xf8/0x170 [<0000000019932b6c>] __sys_sendmsg+0xd3/0x190 [<00000000643ac172>] do_syscall_64+0x37/0x90 [<000000009b79d6dc>] entry_SYSCALL_64_after_hwframe+0x44/0xae Fixes: 63c416887437 ("netlabel: Add network address selectors to the NetLabel/LSM domain mapping") Reported-by: Hulk Robot <redacted> Signed-off-by: Liu Shixin <redacted> --- net/netlabel/netlabel_mgmt.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)diff --git a/net/netlabel/netlabel_mgmt.c b/net/netlabel/netlabel_mgmt.c index e664ab990941..e7f00c0f441e 100644 --- a/net/netlabel/netlabel_mgmt.c +++ b/net/netlabel/netlabel_mgmt.c@@ -191,6 +191,12 @@ static int netlbl_mgmt_add_common(struct genl_info *info, entry->family = AF_INET; entry->def.type = NETLBL_NLTYPE_ADDRSELECT; entry->def.addrsel = addrmap; + + ret_val = netlbl_domhsh_add(entry, audit_info); + if (ret_val != 0) { + kfree(map); + goto add_free_addrmap; + } #if IS_ENABLED(CONFIG_IPV6) } else if (info->attrs[NLBL_MGMT_A_IPV6ADDR]) { struct in6_addr *addr;@@ -243,13 +249,19 @@ static int netlbl_mgmt_add_common(struct genl_info *info, entry->family = AF_INET6; entry->def.type = NETLBL_NLTYPE_ADDRSELECT; entry->def.addrsel = addrmap; + + ret_val = netlbl_domhsh_add(entry, audit_info); + if (ret_val != 0) { + kfree(map); + goto add_free_addrmap; + } #endif /* IPv6 */ + } else { + ret_val = netlbl_domhsh_add(entry, audit_info); + if (ret_val != 0) + goto add_free_addrmap; } - ret_val = netlbl_domhsh_add(entry, audit_info); - if (ret_val != 0) - goto add_free_addrmap; -Hi Shixin, I have a small suggestion about this patch: you can move the variable map out of if/else if branches, like the following code snippet. Be aware to assign the variable map to NULL at first. Then kfree in the last else branch will do nothing. I don't test the following diff, if there are any issues, please let me know.diff --git a/net/netlabel/netlabel_mgmt.c b/net/netlabel/netlabel_mgmt.c index ca52f5085989..1824bcd2272b 100644 --- a/net/netlabel/netlabel_mgmt.c +++ b/net/netlabel/netlabel_mgmt.c@@ -78,6 +78,7 @@ static int netlbl_mgmt_add_common(struct genl_info *info, { int ret_val = -EINVAL; struct netlbl_domaddr_map *addrmap = NULL; + struct netlbl_domaddr4_map *map = NULL; struct cipso_v4_doi *cipsov4 = NULL; #if IS_ENABLED(CONFIG_IPV6) struct calipso_doi *calipso = NULL;@@ -147,7 +148,6 @@ static int netlbl_mgmt_add_common(struct genl_info *info, if (info->attrs[NLBL_MGMT_A_IPV4ADDR]) { struct in_addr *addr; struct in_addr *mask; - struct netlbl_domaddr4_map *map; addrmap = kzalloc(sizeof(*addrmap), GFP_KERNEL); if (addrmap == NULL) {@@ -195,7 +195,6 @@ static int netlbl_mgmt_add_common(struct genl_info *info, } else if (info->attrs[NLBL_MGMT_A_IPV6ADDR]) { struct in6_addr *addr; struct in6_addr *mask; - struct netlbl_domaddr6_map *map; addrmap = kzalloc(sizeof(*addrmap), GFP_KERNEL); if (addrmap == NULL) {@@ -247,8 +246,10 @@ static int netlbl_mgmt_add_common(struct genl_info *info, } ret_val = netlbl_domhsh_add(entry, audit_info); - if (ret_val != 0) + if (ret_val != 0) { + kfree(map); goto add_free_addrmap; + } return 0;The type of map can be struct netlbl_domaddr4_map or struct netlbl_domaddr6_map under different conditions. It seems like I can't put them together simply.
Yes, you're right. It takes more code changes to handle different types. I choose to use the generic void * pointer. The advantage of the diff below is to improve its maintainability of error handling. I add one additional label: add_free_map to handle the deallocation of map. Note that, I did not test this diff. It is only used for clarification. And maybe the compiler complains about the usage of void * pointer.
diff --git a/net/netlabel/netlabel_mgmt.c b/net/netlabel/netlabel_mgmt.c
index ca52f5085989..38edf170b109 100644
--- a/net/netlabel/netlabel_mgmt.c
+++ b/net/netlabel/netlabel_mgmt.c@@ -78,6 +78,7 @@ static int netlbl_mgmt_add_common(struct genl_info *info, { int ret_val = -EINVAL; struct netlbl_domaddr_map *addrmap = NULL; + void *map = NULL; struct cipso_v4_doi *cipsov4 = NULL; #if IS_ENABLED(CONFIG_IPV6) struct calipso_doi *calipso = NULL;
@@ -147,7 +148,6 @@ static int netlbl_mgmt_add_common(struct genl_info *info, if (info->attrs[NLBL_MGMT_A_IPV4ADDR]) { struct in_addr *addr; struct in_addr *mask; - struct netlbl_domaddr4_map *map; addrmap = kzalloc(sizeof(*addrmap), GFP_KERNEL); if (addrmap == NULL) {
@@ -170,7 +170,7 @@ static int netlbl_mgmt_add_common(struct genl_info *info, addr = nla_data(info->attrs[NLBL_MGMT_A_IPV4ADDR]); mask = nla_data(info->attrs[NLBL_MGMT_A_IPV4MASK]); - map = kzalloc(sizeof(*map), GFP_KERNEL); + map = kzalloc(sizeof(struct netlbl_domaddr4_map), GFP_KERNEL); if (map == NULL) { ret_val = -ENOMEM; goto add_free_addrmap;
@@ -183,10 +183,8 @@ static int netlbl_mgmt_add_common(struct genl_info *info, map->def.cipso = cipsov4; ret_val = netlbl_af4list_add(&map->list, &addrmap->list4); - if (ret_val != 0) { - kfree(map); - goto add_free_addrmap; - } + if (ret_val != 0) + goto add_free_map; entry->family = AF_INET; entry->def.type = NETLBL_NLTYPE_ADDRSELECT;
@@ -218,7 +216,7 @@ static int netlbl_mgmt_add_common(struct genl_info *info, addr = nla_data(info->attrs[NLBL_MGMT_A_IPV6ADDR]); mask = nla_data(info->attrs[NLBL_MGMT_A_IPV6MASK]); - map = kzalloc(sizeof(*map), GFP_KERNEL); + map = kzalloc(sizeof(struct netlbl_domaddr6_map), GFP_KERNEL); if (map == NULL) { ret_val = -ENOMEM; goto add_free_addrmap;
@@ -235,10 +233,8 @@ static int netlbl_mgmt_add_common(struct genl_info *info, map->def.calipso = calipso; ret_val = netlbl_af6list_add(&map->list, &addrmap->list6); - if (ret_val != 0) { - kfree(map); - goto add_free_addrmap; - } + if (ret_val != 0) + goto add_free_map; entry->family = AF_INET6; entry->def.type = NETLBL_NLTYPE_ADDRSELECT;
@@ -248,10 +244,12 @@ static int netlbl_mgmt_add_common(struct genl_info *info, ret_val = netlbl_domhsh_add(entry, audit_info); if (ret_val != 0) - goto add_free_addrmap; + goto add_free_map; return 0; +add_free_map: + kfree(map); add_free_addrmap: kfree(addrmap); add_doi_put_def:
Thanks,quoted
quoted
return 0; add_free_addrmap: -- 2.18.0.huawei.25.