linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: fix memory leak in security_sk_alloc()
@ 2022-11-11  9:52 Wang Yufen
  2022-11-11 10:33 ` wangyufen
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Wang Yufen @ 2022-11-11  9:52 UTC (permalink / raw)
  To: linux-security-module, netdev
  Cc: paul, jmorris, serge, martin.lau, daniel, ast, pabeni, kuba,
	edumazet, Wang Yufen, Stanislav Fomichev

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(-)

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;
-- 
1.8.3.1


^ permalink raw reply related	[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: 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  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

* 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

end of thread, other threads:[~2022-11-11 18:53 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).