Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common

From: Dongliang Mu
Date: Wed Jun 09 2021 - 23:21:40 EST


On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin <liushixin2@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Liu Shixin <liushixin2@xxxxxxxxxx>
> ---
> 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
>