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

[Dongliang Mu]

On Fri, Jun 11, 2021 at 7:43 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Wed, Jun 9, 2021 at 9:29 PM 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;
> > -
> >         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