diff mbox series

[-next] netlabel: Fix memory leak in netlbl_mgmt_add_common

Message ID 20210610020108.1356361-1-liushixin2@huawei.com (mailing list archive)
State New
Headers show
Series [-next] netlabel: Fix memory leak in netlbl_mgmt_add_common | expand

Commit Message

Liu Shixin June 10, 2021, 2:01 a.m. UTC
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(-)

Comments

Dongliang Mu June 10, 2021, 3:08 a.m. UTC | #1
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
>
Liu Shixin June 10, 2021, 10:45 a.m. UTC | #2
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
>>
> .
>
Dongliang Mu June 10, 2021, 12:15 p.m. UTC | #3
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
> >>
> > .
> >
>
Paul Moore June 10, 2021, 11:38 p.m. UTC | #4
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).
Dongliang Mu June 11, 2021, 4:05 a.m. UTC | #5
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
diff mbox series

Patch

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: