Messages in this thread | | | From | Dongliang Mu <> | Date | Fri, 11 Jun 2021 12:05:44 +0800 | Subject | Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common |
| |
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
| |