* [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common
@ 2021-06-10 2:01 Liu Shixin
2021-06-10 3:08 ` Dongliang Mu
2021-06-10 23:38 ` Paul Moore
0 siblings, 2 replies; 6+ messages in thread
From: Liu Shixin @ 2021-06-10 2:01 UTC (permalink / raw)
To: Paul Moore, David S. Miller, Jakub Kicinski
Cc: netdev, linux-security-module, linux-kernel, Liu Shixin
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 <hulkci@huawei.com>
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
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;
-
return 0;
add_free_addrmap:
--
2.18.0.huawei.25
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common 2021-06-10 2:01 [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common Liu Shixin @ 2021-06-10 3:08 ` Dongliang Mu 2021-06-10 10:45 ` Liu Shixin 2021-06-10 23:38 ` Paul Moore 1 sibling, 1 reply; 6+ messages in thread From: Dongliang Mu @ 2021-06-10 3:08 UTC (permalink / raw) To: Liu Shixin Cc: Paul Moore, David S. Miller, Jakub Kicinski, netdev, linux-security-module, linux-kernel On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin <liushixin2@huawei.com> wrote: > > 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 <hulkci@huawei.com> > Signed-off-by: Liu Shixin <liushixin2@huawei.com> > --- > 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; > return 0; > > add_free_addrmap: > -- > 2.18.0.huawei.25 > ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common 2021-06-10 3:08 ` Dongliang Mu @ 2021-06-10 10:45 ` Liu Shixin 2021-06-10 12:15 ` Dongliang Mu 0 siblings, 1 reply; 6+ messages in thread From: Liu Shixin @ 2021-06-10 10:45 UTC (permalink / raw) To: Dongliang Mu Cc: Paul Moore, David S. Miller, Jakub Kicinski, netdev, linux-security-module, linux-kernel On 2021/6/10 11:08, Dongliang Mu wrote: > On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin <liushixin2@huawei.com> wrote: >> 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 <hulkci@huawei.com> >> Signed-off-by: Liu Shixin <liushixin2@huawei.com> >> --- >> 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. Thanks, > > > >> return 0; >> >> add_free_addrmap: >> -- >> 2.18.0.huawei.25 >> > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common 2021-06-10 10:45 ` Liu Shixin @ 2021-06-10 12:15 ` Dongliang Mu 0 siblings, 0 replies; 6+ messages in thread From: Dongliang Mu @ 2021-06-10 12:15 UTC (permalink / raw) To: Liu Shixin Cc: Paul Moore, David S. Miller, Jakub Kicinski, netdev, linux-security-module, linux-kernel On Thu, Jun 10, 2021 at 6:45 PM Liu Shixin <liushixin2@huawei.com> wrote: > > On 2021/6/10 11:08, Dongliang Mu wrote: > > On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin <liushixin2@huawei.com> wrote: > >> 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 <hulkci@huawei.com> > >> Signed-off-by: Liu Shixin <liushixin2@huawei.com> > >> --- > >> 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, > > > > > > > >> return 0; > >> > >> add_free_addrmap: > >> -- > >> 2.18.0.huawei.25 > >> > > . > > > ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common 2021-06-10 2:01 [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common Liu Shixin 2021-06-10 3:08 ` Dongliang Mu @ 2021-06-10 23:38 ` Paul Moore 2021-06-11 4:05 ` Dongliang Mu 1 sibling, 1 reply; 6+ messages in thread From: Paul Moore @ 2021-06-10 23:38 UTC (permalink / raw) To: Liu Shixin Cc: David S. Miller, Jakub Kicinski, netdev, linux-security-module, linux-kernel On Wed, Jun 9, 2021 at 9:29 PM Liu Shixin <liushixin2@huawei.com> wrote: > > 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 <hulkci@huawei.com> > Signed-off-by: Liu Shixin <liushixin2@huawei.com> > --- > 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; > - > return 0; Thanks for the report and a fix, although I think there may be a simpler fix that results in less code duplication; some quick pseudo code below: int netlbl_mgmt_add_common(...) { void *map_p = NULL; if (NLBL_MGMT_A_IPV4ADDR) { struct netlbl_domaddr4_map *map; map_p = map; } else if (NLBL_MGMT_A_IPV6ADDR) { struct netlbl_domaddr4_map *map; map_p = map; } add_free_addrmap: kfree(map_p); kfree(addrmap); } ... this approach would even simplify the error handling after the netlbl_af{4,6}list_add() calls a bit too (you could jump straight to add_free_addrmap). -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common 2021-06-10 23:38 ` Paul Moore @ 2021-06-11 4:05 ` Dongliang Mu 0 siblings, 0 replies; 6+ messages in thread From: Dongliang Mu @ 2021-06-11 4:05 UTC (permalink / raw) To: Paul Moore Cc: Liu Shixin, David S. Miller, Jakub Kicinski, netdev, linux-security-module, linux-kernel On Fri, Jun 11, 2021 at 7:43 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jun 9, 2021 at 9:29 PM Liu Shixin <liushixin2@huawei.com> wrote: > > > > 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 <hulkci@huawei.com> > > Signed-off-by: Liu Shixin <liushixin2@huawei.com> > > --- > > 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; > > - > > return 0; > > Thanks for the report and a fix, although I think there may be a > simpler fix that results in less code duplication; some quick pseudo > code below: > > int netlbl_mgmt_add_common(...) > { > void *map_p = NULL; > > if (NLBL_MGMT_A_IPV4ADDR) { > struct netlbl_domaddr4_map *map; > map_p = map; It's better to use a separate map_p pointer, not like the draft patch I sent yesterday. > > } else if (NLBL_MGMT_A_IPV6ADDR) { > struct netlbl_domaddr4_map *map; > map_p = map; > } > > add_free_addrmap: > kfree(map_p); > kfree(addrmap); > } Simple comment here: we should separate kfree(map_p) and kfree(addrmap) into different goto labels, just like the draft patch I sent yesterday. > > ... this approach would even simplify the error handling after the > netlbl_af{4,6}list_add() calls a bit too (you could jump straight to > add_free_addrmap). > > -- > paul moore > www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-11 4:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-10 2:01 [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common Liu Shixin 2021-06-10 3:08 ` Dongliang Mu 2021-06-10 10:45 ` Liu Shixin 2021-06-10 12:15 ` Dongliang Mu 2021-06-10 23:38 ` Paul Moore 2021-06-11 4:05 ` Dongliang Mu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).