From: Liu Shixin <liushixin2@huawei.com>
To: Dongliang Mu <mudongliangabcd@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, <netdev@vger.kernel.org>,
<linux-security-module@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common
Date: Thu, 10 Jun 2021 18:45:15 +0800 [thread overview]
Message-ID: <ea1c6878-94d4-63ba-5dea-1190c146581d@huawei.com> (raw)
In-Reply-To: <CAD-N9QWypyEa65-sz3rrtM2o5xzQd_5kJPyC4n+nK5JTviQvEQ@mail.gmail.com>
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
>>
> .
>
next prev parent reply other threads:[~2021-06-10 10:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-06-10 12:15 ` Dongliang Mu
2021-06-10 23:38 ` Paul Moore
2021-06-11 4:05 ` Dongliang Mu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ea1c6878-94d4-63ba-5dea-1190c146581d@huawei.com \
--to=liushixin2@huawei.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mudongliangabcd@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=paul@paul-moore.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).