* [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
@ 2019-03-07 2:57 Mao Wenan
2019-03-07 20:36 ` Paul Moore
0 siblings, 1 reply; 5+ messages in thread
From: Mao Wenan @ 2019-03-07 2:57 UTC (permalink / raw)
To: davem, netdev, paul, sds, eparis, selinux, linux-kernel
If netlbl_sock_setattr() is failed, it directly returns rc and forgets
to free secattr.
BUG: memory leak
unreferenced object 0xffff8883c3ea4200 (size 2664):
comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
hex dump (first 32 bytes):
7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...........qN$..
02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@............
backtrace:
[<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
[<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
[<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
[<000000004274b384>] sock_create net/socket.c:1315 [inline]
[<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
[<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
[<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
[<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
[<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
[<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[<00000000f737e62f>] 0xffffffffffffffff
BUG: memory leak
unreferenced object 0xffff8883de23d570 (size 32):
comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
hex dump (first 32 bytes):
02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff ........`.@.....
01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00 ................
backtrace:
[<0000000035ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
[<00000000302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
[<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
[<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
[<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
[<000000004274b384>] sock_create net/socket.c:1315 [inline]
[<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
[<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
[<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
[<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
[<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
[<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[<00000000f737e62f>] 0xffffffffffffffff
BUG: memory leak
unreferenced object 0xffff8883ce40a960 (size 64):
comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<0000000029add401>] selinux_netlbl_socket_post_create+0x68/0x130 security/selinux/netlabel.c:416
[<000000005368a19c>] selinux_socket_post_create+0x31a/0x7f0 security/selinux/hooks.c:4597
[<00000000bd4730e2>] security_socket_post_create+0x70/0xc0 security/security.c:1385
[<00000000671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
[<000000004274b384>] sock_create net/socket.c:1315 [inline]
[<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
[<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
[<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
[<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
[<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
[<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[<00000000f737e62f>] 0xffffffffffffffff
Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in SELinux")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
security/selinux/netlabel.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index 186e727b737b..f3da05338580 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
rc = 0;
break;
}
+ if (rc != 0) {
+ netlbl_secattr_free(secattr);
+ }
return rc;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
2019-03-07 2:57 [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create() Mao Wenan
@ 2019-03-07 20:36 ` Paul Moore
2019-03-08 6:31 ` maowenan
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2019-03-07 20:36 UTC (permalink / raw)
To: Mao Wenan
Cc: davem, netdev, Stephen Smalley, Eric Paris, selinux, linux-kernel
On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan <maowenan@huawei.com> wrote:
>
> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
> to free secattr.
>
> BUG: memory leak
> unreferenced object 0xffff8883c3ea4200 (size 2664):
> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> hex dump (first 32 bytes):
> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...........qN$..
> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@............
> backtrace:
> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<00000000f737e62f>] 0xffffffffffffffff
>
> BUG: memory leak
> unreferenced object 0xffff8883de23d570 (size 32):
> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> hex dump (first 32 bytes):
> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff ........`.@.....
> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00 ................
> backtrace:
> [<0000000035ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
> [<00000000302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<00000000f737e62f>] 0xffffffffffffffff
>
> BUG: memory leak
> unreferenced object 0xffff8883ce40a960 (size 64):
> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<0000000029add401>] selinux_netlbl_socket_post_create+0x68/0x130 security/selinux/netlabel.c:416
> [<000000005368a19c>] selinux_socket_post_create+0x31a/0x7f0 security/selinux/hooks.c:4597
> [<00000000bd4730e2>] security_socket_post_create+0x70/0xc0 security/security.c:1385
> [<00000000671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<00000000f737e62f>] 0xffffffffffffffff
>
> Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in SELinux")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> security/selinux/netlabel.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 186e727b737b..f3da05338580 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
> rc = 0;
> break;
> }
> + if (rc != 0) {
> + netlbl_secattr_free(secattr);
> + }
This is likely going to cause a problem as the
sock->sk_security->nlbl_secattr still has a reference to the secattr
pointer you are releasing here. Assuming things are working correctly
elsewhere, I believe freeing secattr here will result in a double free
when the network stacks cleans up after the failed socket creation
(via the sock_release() call in the error handling code).
It looks like you may have found this via a test tool (syzbot?), do
you have a reproducer you can share?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
2019-03-07 20:36 ` Paul Moore
@ 2019-03-08 6:31 ` maowenan
2019-03-08 13:20 ` Paul Moore
0 siblings, 1 reply; 5+ messages in thread
From: maowenan @ 2019-03-08 6:31 UTC (permalink / raw)
To: Paul Moore
Cc: davem, netdev, Stephen Smalley, Eric Paris, selinux, linux-kernel
On 2019/3/8 4:36, Paul Moore wrote:
> On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan <maowenan@huawei.com> wrote:
>>
>> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
>> to free secattr.
>>
>> BUG: memory leak
>> unreferenced object 0xffff8883c3ea4200 (size 2664):
>> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
>> hex dump (first 32 bytes):
>> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...........qN$..
>> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@............
>> backtrace:
>> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
>> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
>> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
>> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
>> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
>> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
>> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
>> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
>> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [<00000000f737e62f>] 0xffffffffffffffff
>>
>> BUG: memory leak
>> unreferenced object 0xffff8883de23d570 (size 32):
>> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
>> hex dump (first 32 bytes):
>> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff ........`.@.....
>> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00 ................
>> backtrace:
>> [<0000000035ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
>> [<00000000302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
>> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
>> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
>> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
>> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
>> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
>> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
>> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
>> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
>> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [<00000000f737e62f>] 0xffffffffffffffff
>>
>> BUG: memory leak
>> unreferenced object 0xffff8883ce40a960 (size 64):
>> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
>> hex dump (first 32 bytes):
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> backtrace:
>> [<0000000029add401>] selinux_netlbl_socket_post_create+0x68/0x130 security/selinux/netlabel.c:416
>> [<000000005368a19c>] selinux_socket_post_create+0x31a/0x7f0 security/selinux/hooks.c:4597
>> [<00000000bd4730e2>] security_socket_post_create+0x70/0xc0 security/security.c:1385
>> [<00000000671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
>> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
>> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
>> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
>> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
>> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
>> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [<00000000f737e62f>] 0xffffffffffffffff
>>
>> Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in SELinux")
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> ---
>> security/selinux/netlabel.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
>> index 186e727b737b..f3da05338580 100644
>> --- a/security/selinux/netlabel.c
>> +++ b/security/selinux/netlabel.c
>> @@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
>> rc = 0;
>> break;
>> }
>> + if (rc != 0) {
>> + netlbl_secattr_free(secattr);
>> + }
>
> This is likely going to cause a problem as the
> sock->sk_security->nlbl_secattr still has a reference to the secattr
> pointer you are releasing here. Assuming things are working correctly
> elsewhere, I believe freeing secattr here will result in a double free
> when the network stacks cleans up after the failed socket creation
> (via the sock_release() call in the error handling code).
Do you mean here?
err = security_socket_post_create(sock, family, type, protocol, kern);
if (err)
goto out_sock_release;
...
out_sock_release:
sock_release(sock);
return err;
yes, the call trace may like below:
sock_release->__sock_release->inet_release->tcp_close->sock_put->sk_free->__sk_free->sk_destruct->
__sk_destruct->sk_prot_free->security_sk_free->selinux_sk_free_security->selinux_netlbl_sk_security_free->
netlbl_secattr_free
so, it is no need to free in selinux_netlbl_socket_post_create().
Please correct me if this is incorrect.
>
> It looks like you may have found this via a test tool (syzbot?), do
> you have a reproducer you can share?
>
The issue has been found by syzkaller, but unfortunately it is no reproducer, it is
only some backtrace.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
2019-03-08 6:31 ` maowenan
@ 2019-03-08 13:20 ` Paul Moore
2019-03-08 13:39 ` maowenan
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2019-03-08 13:20 UTC (permalink / raw)
To: maowenan
Cc: davem, netdev, Stephen Smalley, Eric Paris, selinux, linux-kernel
On Fri, Mar 8, 2019 at 1:31 AM maowenan <maowenan@huawei.com> wrote:
> On 2019/3/8 4:36, Paul Moore wrote:
> > On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan <maowenan@huawei.com> wrote:
> >>
> >> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
> >> to free secattr.
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff8883c3ea4200 (size 2664):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >> hex dump (first 32 bytes):
> >> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...........qN$..
> >> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@............
> >> backtrace:
> >> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> >> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> >> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<00000000f737e62f>] 0xffffffffffffffff
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff8883de23d570 (size 32):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >> hex dump (first 32 bytes):
> >> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff ........`.@.....
> >> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00 ................
> >> backtrace:
> >> [<0000000035ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
> >> [<00000000302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
> >> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> >> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> >> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<00000000f737e62f>] 0xffffffffffffffff
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff8883ce40a960 (size 64):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
> >> hex dump (first 32 bytes):
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> backtrace:
> >> [<0000000029add401>] selinux_netlbl_socket_post_create+0x68/0x130 security/selinux/netlabel.c:416
> >> [<000000005368a19c>] selinux_socket_post_create+0x31a/0x7f0 security/selinux/hooks.c:4597
> >> [<00000000bd4730e2>] security_socket_post_create+0x70/0xc0 security/security.c:1385
> >> [<00000000671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
> >> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> >> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> >> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<00000000f737e62f>] 0xffffffffffffffff
> >>
> >> Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in SELinux")
> >> Reported-by: Hulk Robot <hulkci@huawei.com>
> >> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> >> ---
> >> security/selinux/netlabel.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> >> index 186e727b737b..f3da05338580 100644
> >> --- a/security/selinux/netlabel.c
> >> +++ b/security/selinux/netlabel.c
> >> @@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
> >> rc = 0;
> >> break;
> >> }
> >> + if (rc != 0) {
> >> + netlbl_secattr_free(secattr);
> >> + }
> >
> > This is likely going to cause a problem as the
> > sock->sk_security->nlbl_secattr still has a reference to the secattr
> > pointer you are releasing here. Assuming things are working correctly
> > elsewhere, I believe freeing secattr here will result in a double free
> > when the network stacks cleans up after the failed socket creation
> > (via the sock_release() call in the error handling code).
> Do you mean here?
> err = security_socket_post_create(sock, family, type, protocol, kern);
> if (err)
> goto out_sock_release;
> ...
>
> out_sock_release:
> sock_release(sock);
> return err;
> yes, the call trace may like below:
> sock_release->__sock_release->inet_release->tcp_close->sock_put->sk_free->__sk_free->sk_destruct->
> __sk_destruct->sk_prot_free->security_sk_free->selinux_sk_free_security->selinux_netlbl_sk_security_free->
> netlbl_secattr_free
>
> so, it is no need to free in selinux_netlbl_socket_post_create().
> Please correct me if this is incorrect.
I haven't followed the entirely call stack lately, but that looks right.
> > It looks like you may have found this via a test tool (syzbot?), do
> > you have a reproducer you can share?
>
> The issue has been found by syzkaller, but unfortunately it is no reproducer, it is
> only some backtrace.
If you are able to reproduce this please let us know, it may be a sign
that something is wrong with the LSM hooks.
Thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
2019-03-08 13:20 ` Paul Moore
@ 2019-03-08 13:39 ` maowenan
0 siblings, 0 replies; 5+ messages in thread
From: maowenan @ 2019-03-08 13:39 UTC (permalink / raw)
To: Paul Moore
Cc: davem, netdev, Stephen Smalley, Eric Paris, selinux, linux-kernel
On 2019/3/8 21:20, Paul Moore wrote:
> On Fri, Mar 8, 2019 at 1:31 AM maowenan <maowenan@huawei.com> wrote:
>> On 2019/3/8 4:36, Paul Moore wrote:
>>> On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan <maowenan@huawei.com> wrote:
>>>>
>>>> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
>>>> to free secattr.
>>>>
>>>> BUG: memory leak
>>>> unreferenced object 0xffff8883c3ea4200 (size 2664):
>>>> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
>>>> hex dump (first 32 bytes):
>>>> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...........qN$..
>>>> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@............
>>>> backtrace:
>>>> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
>>>> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
>>>> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
>>>> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
>>>> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
>>>> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
>>>> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
>>>> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
>>>> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>>>> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>>> [<00000000f737e62f>] 0xffffffffffffffff
>>>>
>>>> BUG: memory leak
>>>> unreferenced object 0xffff8883de23d570 (size 32):
>>>> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
>>>> hex dump (first 32 bytes):
>>>> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff ........`.@.....
>>>> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00 ................
>>>> backtrace:
>>>> [<0000000035ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
>>>> [<00000000302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
>>>> [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
>>>> [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
>>>> [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
>>>> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
>>>> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
>>>> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
>>>> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
>>>> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
>>>> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>>>> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>>> [<00000000f737e62f>] 0xffffffffffffffff
>>>>
>>>> BUG: memory leak
>>>> unreferenced object 0xffff8883ce40a960 (size 64):
>>>> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
>>>> hex dump (first 32 bytes):
>>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>>> backtrace:
>>>> [<0000000029add401>] selinux_netlbl_socket_post_create+0x68/0x130 security/selinux/netlabel.c:416
>>>> [<000000005368a19c>] selinux_socket_post_create+0x31a/0x7f0 security/selinux/hooks.c:4597
>>>> [<00000000bd4730e2>] security_socket_post_create+0x70/0xc0 security/security.c:1385
>>>> [<00000000671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
>>>> [<000000004274b384>] sock_create net/socket.c:1315 [inline]
>>>> [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
>>>> [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
>>>> [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
>>>> [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
>>>> [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>>>> [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>>> [<00000000f737e62f>] 0xffffffffffffffff
>>>>
>>>> Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in SELinux")
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>>>> ---
>>>> security/selinux/netlabel.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
>>>> index 186e727b737b..f3da05338580 100644
>>>> --- a/security/selinux/netlabel.c
>>>> +++ b/security/selinux/netlabel.c
>>>> @@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
>>>> rc = 0;
>>>> break;
>>>> }
>>>> + if (rc != 0) {
>>>> + netlbl_secattr_free(secattr);
>>>> + }
>>>
>>> This is likely going to cause a problem as the
>>> sock->sk_security->nlbl_secattr still has a reference to the secattr
>>> pointer you are releasing here. Assuming things are working correctly
>>> elsewhere, I believe freeing secattr here will result in a double free
>>> when the network stacks cleans up after the failed socket creation
>>> (via the sock_release() call in the error handling code).
>> Do you mean here?
>> err = security_socket_post_create(sock, family, type, protocol, kern);
>> if (err)
>> goto out_sock_release;
>> ...
>>
>> out_sock_release:
>> sock_release(sock);
>> return err;
>> yes, the call trace may like below:
>> sock_release->__sock_release->inet_release->tcp_close->sock_put->sk_free->__sk_free->sk_destruct->
>> __sk_destruct->sk_prot_free->security_sk_free->selinux_sk_free_security->selinux_netlbl_sk_security_free->
>> netlbl_secattr_free
>>
>> so, it is no need to free in selinux_netlbl_socket_post_create().
>> Please correct me if this is incorrect.
>
> I haven't followed the entirely call stack lately, but that looks right.
>
>>> It looks like you may have found this via a test tool (syzbot?), do
>>> you have a reproducer you can share?
>>
>> The issue has been found by syzkaller, but unfortunately it is no reproducer, it is
>> only some backtrace.
>
> If you are able to reproduce this please let us know, it may be a sign
> that something is wrong with the LSM hooks.
>
sure, I try to reproduce it and keep running test the whole night.
> Thanks.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-08 13:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07 2:57 [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create() Mao Wenan
2019-03-07 20:36 ` Paul Moore
2019-03-08 6:31 ` maowenan
2019-03-08 13:20 ` Paul Moore
2019-03-08 13:39 ` maowenan
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).