* Re: [PATCH] net: fix memory leak in security_sk_alloc()
2022-11-11 9:52 [PATCH] net: fix memory leak in security_sk_alloc() Wang Yufen
@ 2022-11-11 10:33 ` wangyufen
2022-11-11 15:08 ` Paul Moore
2022-11-11 16:28 ` Eric Dumazet
2 siblings, 0 replies; 5+ messages in thread
From: wangyufen @ 2022-11-11 10:33 UTC (permalink / raw)
To: linux-security-module, netdev
Cc: paul, jmorris, serge, martin.lau, daniel, ast, pabeni, kuba,
edumazet, Stanislav Fomichev
在 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;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] net: fix memory leak in security_sk_alloc()
2022-11-11 9:52 [PATCH] net: fix memory leak in security_sk_alloc() Wang Yufen
2022-11-11 10:33 ` wangyufen
@ 2022-11-11 15:08 ` Paul Moore
2022-11-11 18:52 ` Stanislav Fomichev
2022-11-11 16:28 ` Eric Dumazet
2 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2022-11-11 15:08 UTC (permalink / raw)
To: Wang Yufen
Cc: linux-security-module, netdev, jmorris, serge, martin.lau, daniel,
ast, pabeni, kuba, edumazet, Stanislav Fomichev
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.
--
paul-moore.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: fix memory leak in security_sk_alloc()
2022-11-11 15:08 ` Paul Moore
@ 2022-11-11 18:52 ` Stanislav Fomichev
0 siblings, 0 replies; 5+ messages in thread
From: Stanislav Fomichev @ 2022-11-11 18:52 UTC (permalink / raw)
To: Paul Moore
Cc: Wang Yufen, linux-security-module, netdev, jmorris, serge,
martin.lau, daniel, ast, pabeni, kuba, edumazet
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: fix memory leak in security_sk_alloc()
2022-11-11 9:52 [PATCH] net: fix memory leak in security_sk_alloc() Wang Yufen
2022-11-11 10:33 ` wangyufen
2022-11-11 15:08 ` Paul Moore
@ 2022-11-11 16:28 ` Eric Dumazet
2 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-11-11 16:28 UTC (permalink / raw)
To: Wang Yufen
Cc: linux-security-module, netdev, paul, jmorris, serge, martin.lau,
daniel, ast, pabeni, kuba, Stanislav Fomichev
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread