Re: [PATCH bpf] selftests/bpf: fix memory leak of lsm_cgroup

[sdf@google.com]

On 11/14, Wang Yufen 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

> 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. To fix, skip the
> "EPERM" test when it runs in the environments that already have
> non-bpf lsms installed

> Fixes: dca85aac8895 ("selftests/bpf: lsm_cgroup functional test")
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> Cc: Stanislav Fomichev <sdf@google.com>
> ---
>   tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c | 19  
> +++++++++++++++----
>   tools/testing/selftests/bpf/progs/lsm_cgroup.c      |  8 ++++++++
>   2 files changed, 23 insertions(+), 4 deletions(-)

> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c  
> b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> index 1102e4f..a927ade 100644
> --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> @@ -173,10 +173,14 @@ static void test_lsm_cgroup_functional(void)
>   	ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 4, "total prog count");
>   	ASSERT_EQ(query_prog_cnt(cgroup_fd2, NULL), 1, "total prog count");

> -	/* AF_UNIX is prohibited. */
> -
>   	fd = socket(AF_UNIX, SOCK_STREAM, 0);
> -	ASSERT_LT(fd, 0, "socket(AF_UNIX)");
> +	if (skel->kconfig->CONFIG_SECURITY_APPARMOR
> +	    || skel->kconfig->CONFIG_SECURITY_SELINUX
> +	    || skel->kconfig->CONFIG_SECURITY_SMACK)

[..]

> +		ASSERT_GE(fd, 0, "socket(AF_UNIX)");

nit: maybe skip this completely instead of having ASSERT_GE+close?

	if (!(skel->kconfig->CONFIG_SECURITY_APPARMOR || _SELINUX || _SMACK)
		/* AF_UNIX is prohibited. */
		ASSERT_LT(fd, 0, "socket(AF_UNIX)");


> +	else
> +		/* AF_UNIX is prohibited. */
> +		ASSERT_LT(fd, 0, "socket(AF_UNIX)");
>   	close(fd);

>   	/* AF_INET6 gets default policy (sk_priority). */
> @@ -233,11 +237,18 @@ static void test_lsm_cgroup_functional(void)

>   	/* AF_INET6+SOCK_STREAM
>   	 * AF_PACKET+SOCK_RAW
> +	 * AF_UNIX+SOCK_RAW if already have non-bpf lsms installed
>   	 * listen_fd
>   	 * client_fd
>   	 * accepted_fd
>   	 */
> -	ASSERT_EQ(skel->bss->called_socket_post_create2, 5, "called_create2");
> +	if (skel->kconfig->CONFIG_SECURITY_APPARMOR
> +	    || skel->kconfig->CONFIG_SECURITY_SELINUX
> +	    || skel->kconfig->CONFIG_SECURITY_SMACK)
> +		/* AF_UNIX+SOCK_RAW if already have non-bpf lsms installed */
> +		ASSERT_EQ(skel->bss->called_socket_post_create2, 6, "called_create2");
> +	else
> +		ASSERT_EQ(skel->bss->called_socket_post_create2, 5, "called_create2");

>   	/* start_server
>   	 * bind(ETH_P_ALL)
> diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup.c  
> b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
> index 4f2d60b..02c11d1 100644
> --- a/tools/testing/selftests/bpf/progs/lsm_cgroup.c
> +++ b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
> @@ -7,6 +7,10 @@

>   char _license[] SEC("license") = "GPL";

> +extern bool CONFIG_SECURITY_SELINUX __kconfig __weak;
> +extern bool CONFIG_SECURITY_SMACK __kconfig __weak;
> +extern bool CONFIG_SECURITY_APPARMOR __kconfig __weak;
> +
>   #ifndef AF_PACKET
>   #define AF_PACKET 17
>   #endif
> @@ -140,6 +144,10 @@ int BPF_PROG(socket_bind2, struct socket *sock,  
> struct sockaddr *address,
>   int BPF_PROG(socket_alloc, struct sock *sk, int family, gfp_t priority)
>   {
>   	called_socket_alloc++;
> +	/* if already have non-bpf lsms installed, EPERM will cause memory leak  
> of non-bpf lsms */
> +	if (CONFIG_SECURITY_SELINUX || CONFIG_SECURITY_SMACK ||  
> CONFIG_SECURITY_APPARMOR)
> +		return 1;
> +
>   	if (family == AF_UNIX)
>   		return 0; /* EPERM */

> --
> 1.8.3.1