| Message ID | 1668160371-39153-1-git-send-email-wangyufen@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fix memory leak in security_sk_alloc() | expand |
在 2022/11/11 17:52, Wang Yufen 写道: > 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. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") The Fixes tag is incorrect, change to Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor") > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > Cc: Stanislav Fomichev <sdf@google.com> > --- > net/core/sock.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index a3ba035..e457a9d 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -2030,8 +2030,11 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority, > sk = kmalloc(prot->obj_size, priority); > > if (sk != NULL) { sk->sk_security is not initialized, add "sk->sk_security = NULL;" here. > - if (security_sk_alloc(sk, family, priority)) > + if (security_sk_alloc(sk, family, priority)) { > + if (sk->sk_security) > + goto out_free_sec; > goto out_free; > + } > > if (!try_module_get(prot->owner)) > goto out_free_sec;
On Fri, Nov 11, 2022 at 4:32 AM Wang Yufen <wangyufen@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.
On Fri, Nov 11, 2022 at 1:32 AM Wang Yufen <wangyufen@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. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Really the bug has not been added in linux-2.6.12, but this year with bpf lsm ... > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > Cc: Stanislav Fomichev <sdf@google.com> > --- > net/core/sock.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index a3ba035..e457a9d 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -2030,8 +2030,11 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority, > sk = kmalloc(prot->obj_size, priority); > > if (sk != NULL) { > - if (security_sk_alloc(sk, family, priority)) > + if (security_sk_alloc(sk, family, priority)) { This does not make sense. A proper fix should be in security_sk_alloc(), not in callers. (Even if there is one caller today,) > + if (sk->sk_security) > + goto out_free_sec; > goto out_free; > + } > > if (!try_module_get(prot->owner)) > goto out_free_sec; > -- > 1.8.3.1 >
On Fri, Nov 11, 2022 at 7:08 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Nov 11, 2022 at 4:32 AM Wang Yufen <wangyufen@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
diff --git a/net/core/sock.c b/net/core/sock.c index a3ba035..e457a9d 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -2030,8 +2030,11 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority, sk = kmalloc(prot->obj_size, priority); if (sk != NULL) { - if (security_sk_alloc(sk, family, priority)) + if (security_sk_alloc(sk, family, priority)) { + if (sk->sk_security) + goto out_free_sec; goto out_free; + } if (!try_module_get(prot->owner)) goto out_free_sec;
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. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Wang Yufen <wangyufen@huawei.com> Cc: Stanislav Fomichev <sdf@google.com> --- net/core/sock.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)