[PATCH] net: fix memory leak in security_sk_alloc()

[Stanislav Fomichev]

On Fri, Nov 11, 2022 at 7:08 AM Paul Moore <paul at paul-moore.com> wrote:
>
> On Fri, Nov 11, 2022 at 4:32 AM Wang Yufen <wangyufen at huawei.com> wrote:
> >
> > kmemleak reports this issue:
> >
> > unreferenced object 0xffff88810b7835c0 (size 32):
> >   comm "test_progs", pid 270, jiffies 4294969007 (age 1621.315s)
> >   hex dump (first 32 bytes):
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >     03 00 00 00 03 00 00 00 0f 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<00000000376cdeab>] kmalloc_trace+0x27/0x110
> >     [<000000003bcdb3b6>] selinux_sk_alloc_security+0x66/0x110
> >     [<000000003959008f>] security_sk_alloc+0x47/0x80
> >     [<00000000e7bc6668>] sk_prot_alloc+0xbd/0x1a0
> >     [<0000000002d6343a>] sk_alloc+0x3b/0x940
> >     [<000000009812a46d>] unix_create1+0x8f/0x3d0
> >     [<000000005ed0976b>] unix_create+0xa1/0x150
> >     [<0000000086a1d27f>] __sock_create+0x233/0x4a0
> >     [<00000000cffe3a73>] __sys_socket_create.part.0+0xaa/0x110
> >     [<0000000007c63f20>] __sys_socket+0x49/0xf0
> >     [<00000000b08753c8>] __x64_sys_socket+0x42/0x50
> >     [<00000000b56e26b3>] do_syscall_64+0x3b/0x90
> >     [<000000009b4871b8>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >
> > The issue occurs in the following scenarios:
> >
> > unix_create1()
> >   sk_alloc()
> >     sk_prot_alloc()
> >       security_sk_alloc()
> >         call_int_hook()
> >           hlist_for_each_entry()
> >             entry1->hook.sk_alloc_security
> >             <-- selinux_sk_alloc_security() succeeded,
> >             <-- sk->security alloced here.
> >             entry2->hook.sk_alloc_security
> >             <-- bpf_lsm_sk_alloc_security() failed
> >       goto out_free;
> >         ...    <-- the sk->security not freed, memleak
> >
> > To fix, if security_sk_alloc() failed and sk->security not null,
> > goto out_free_sec to reclaim resources.
> >
> > I'm not sure whether this fix makes sense, but if hook lists don't
> > support this usage, might need to modify the
> > "tools/testing/selftests/bpf/progs/lsm_cgroup.c" test case.
>
> The core problem is that the LSM is not yet fully stacked (work is
> actively going on in this space) which means that some LSM hooks do
> not support multiple LSMs at the same time; unfortunately the
> networking hooks fall into this category.
>
> While there can only be one LSM which manages the sock::sk_security
> field by defining a sk_alloc_security hook, it *should* be possible
> for other LSMs to to leverage the socket hooks, e.g.
> security_socket_bind(), as long as they don't manipulate any of the
> sock::sk_security state.
>
> I would suggest modifying the ".../bpf/progs/lsm_cgroup.c" test until
> the LSM supports stacking the networking hooks.

Agreed. Let's add some code to skip the test when it runs in the
environments that already have non-bpf lsms installed?

> --
> paul-moore.com