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 <[email protected]>
Signed-off-by: Liu Shixin <[email protected]>
---
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
On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin <[email protected]> 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 <[email protected]>
> Signed-off-by: Liu Shixin <[email protected]>
> ---
> 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
>
On 2021/6/10 11:08, Dongliang Mu wrote:
> On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin <[email protected]> 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 <[email protected]>
>> Signed-off-by: Liu Shixin <[email protected]>
>> ---
>> 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
>>
> .
>
On Thu, Jun 10, 2021 at 6:45 PM Liu Shixin <[email protected]> wrote:
>
> On 2021/6/10 11:08, Dongliang Mu wrote:
> > On Thu, Jun 10, 2021 at 9:31 AM Liu Shixin <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Liu Shixin <[email protected]>
> >> ---
> >> 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
> >>
> > .
> >
>
On Wed, Jun 9, 2021 at 9:29 PM Liu Shixin <[email protected]> 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 <[email protected]>
> Signed-off-by: Liu Shixin <[email protected]>
> ---
> 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
http://www.paul-moore.com
On Fri, Jun 11, 2021 at 7:43 AM Paul Moore <[email protected]> wrote:
>
> On Wed, Jun 9, 2021 at 9:29 PM Liu Shixin <[email protected]> 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 <[email protected]>
> > Signed-off-by: Liu Shixin <[email protected]>
> > ---
> > 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
> http://www.paul-moore.com