lkml.org 
[lkml]   [2021]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH -next] netlabel: Fix memory leak in netlbl_mgmt_add_common
    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
    > >>
    > > .
    > >
    >

    \
     
     \ /
      Last update: 2021-06-10 14:17    [W:7.197 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site