| Message ID | 20190307025731.49369-1-maowenan@huawei.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create() | expand |
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?
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.
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.
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. >
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; }
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(+)