[PATCH] general protection fault in sock_has

Discussion:

[PATCH] general protection fault in sock_has_perm

Paul Moore

2018-01-18 22:36:38 UTC

general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
task: ffff8801d1095f00 task.stack: ffff8800b5950000
RIP: 0010:[<ffffffff81b69b7e>] [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
RSP: 0018:ffff8800b5957ce0 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 1ffff10016b2af9f RCX: ffffffff81b69b51
RDX: 0000000000000002 RSI: 0000000000000000 RDI: 0000000000000010
RBP: ffff8800b5957de0 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: 1ffff10016b2af68 R12: ffff8800b5957db8
R13: 0000000000000000 R14: ffff8800b7259f40 R15: 00000000000000d7
FS: 00007f72f5ae2700(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000a2fa38 CR3: 00000001d7980000 CR4: 0000000000160670
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30 0000000041b58ab3
ffffffff83fc82f2 ffffffff81b69980 0000000000000246 ffff8801d1096770
ffff8801d3165668 ffffffff8157844b ffff8801d1095f00
ffff880000000001
[<ffffffff81b6a19d>] selinux_socket_setsockopt+0x4d/0x80 security/selinux/hooks.c:4338
[<ffffffff81b4873d>] security_socket_setsockopt+0x7d/0xb0 security/security.c:1257
[<ffffffff82df1ac8>] SYSC_setsockopt net/socket.c:1757 [inline]
[<ffffffff82df1ac8>] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[<ffffffff83776499>] entry_SYSCALL_64_fastpath+0x16/0x92
Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
00 41 8b 75 10 31
RIP [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
RSP <ffff8800b5957ce0>
---[ end trace 7b5aaf788fef6174 ]---
In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
flag") and all the associated infrastructure changes to take advantage
of a RCU grace period before freeing, there is a heightened
possibility that a security check is performed while an ill-timed
setsockopt call races in from user space. It then is prudent to null
check sk_security, and if the case, reject the permissions.
This adjustment is orthogonal to infrastructure improvements that may
nullify the needed check, but should be added as good code hygiene.

I'm skeptical that this is the full solution for systems that lack the
SOCK_RCU_FREE protection. Is this really limited to just
setsockopt()?

---
This patch should be applied to all stable trees (author wants
minimum of 3.18, 4.4, 4.9 and 4.14)
security/selinux/hooks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..95d7c8143373 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4342,7 +4342,7 @@ static int sock_has_perm(struct sock *sk, u32 perms)
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
- if (sksec->sid == SECINITSID_KERNEL)
+ if (!sksec || sksec->sid == SECINITSID_KERNEL)
return 0;
ad.type = LSM_AUDIT_DATA_NET;
--
2.16.0.rc1.238.g530d649a79-goog

--
paul moore
www.paul-moore.com

Greg KH

2018-01-19 07:48:12 UTC

Permalink

Note, if you want this type of thing to show up in the patch itself, so
I will see it when it hits Linus's tree, you can just change the stable
line to be:
cc: stable <***@vger.kernel.org> # 3.18+

thanks,

greg k-h

Mark Salyzyn

2018-01-19 15:49:58 UTC

Permalink

Post by Paul Moore

general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
. . .
[<ffffffff81b6a19d>] selinux_socket_setsockopt+0x4d/0x80 security/selinux/hooks.c:4338
[<ffffffff81b4873d>] security_socket_setsockopt+0x7d/0xb0 security/security.c:1257
[<ffffffff82df1ac8>] SYSC_setsockopt net/socket.c:1757 [inline]
[<ffffffff82df1ac8>] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[<ffffffff83776499>] entry_SYSCALL_64_fastpath+0x16/0x92
Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
00 41 8b 75 10 31
RIP [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
RSP <ffff8800b5957ce0>
---[ end trace 7b5aaf788fef6174 ]---
In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
flag") and all the associated infrastructure changes to take advantage
of a RCU grace period before freeing, there is a heightened
possibility that a security check is performed while an ill-timed
setsockopt call races in from user space. It then is prudent to null
check sk_security, and if the case, reject the permissions.
This adjustment is orthogonal to infrastructure improvements that may
nullify the needed check, but should be added as good code hygiene.

I'm skeptical that this is the full solution for systems that lack the
SOCK_RCU_FREE protection. Is this really limited to just
setsockopt()?

Maybe overstepped in my analysis and assumptions?

This is a result of a fuzzer hitting an android 4.4 KASAN kernel. We (so
far) have _not_ seen this with an android 4.9 KASAN kernel (which has
the SOCK_RCU_FREE adjustments). There is no standalone duplication or
PoC _except_ via the fuzzer. The rest of the statements stands based on
this tidbit (statements of general good code hygiene, not 100% sure
SOCK_RCU_FREE usage is completely covered, KISS solution etc).

To be honest, yes, this may be a layer in the onion (swat this NULL
check does not by itself solve the _problem_), I'd prefer kernel
continuing on in a rational manner rather than panic ... and I have a
gut feeling this could be a gratuitous NULL check if all the bugs in the
network layer have been solved <that may be sarcasm, I can not tell>.
Programming to solve a problem with one's gut is not a good practice,
but hygiene is. This is 10 characters, and an estimated 1.2ns of added
hygiene.

No, I do not think this is limited to setsockopt, but would be willing
to believe a multithreaded attack of any socket functions or ioctl would
drop down to the check with sock_has_perm at possibly the wrong time in
socket teardown.

Sincerely -- Mark Salyzyn

Paul Moore

2018-01-19 17:06:40 UTC

Permalink

Post by Mark Salyzyn

Post by Paul Moore

general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
. . .
[<ffffffff81b6a19d>] selinux_socket_setsockopt+0x4d/0x80
security/selinux/hooks.c:4338
[<ffffffff81b4873d>] security_socket_setsockopt+0x7d/0xb0
security/security.c:1257
[<ffffffff82df1ac8>] SYSC_setsockopt net/socket.c:1757 [inline]
[<ffffffff82df1ac8>] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[<ffffffff83776499>] entry_SYSCALL_64_fastpath+0x16/0x92
Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
00 41 8b 75 10 31
RIP [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0
security/selinux/hooks.c:4069
RSP <ffff8800b5957ce0>
---[ end trace 7b5aaf788fef6174 ]---
In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
flag") and all the associated infrastructure changes to take advantage
of a RCU grace period before freeing, there is a heightened
possibility that a security check is performed while an ill-timed
setsockopt call races in from user space. It then is prudent to null
check sk_security, and if the case, reject the permissions.
This adjustment is orthogonal to infrastructure improvements that may
nullify the needed check, but should be added as good code hygiene.

I'm skeptical that this is the full solution for systems that lack the
SOCK_RCU_FREE protection. Is this really limited to just
setsockopt()?

Maybe overstepped in my analysis and assumptions?
This is a result of a fuzzer hitting an android 4.4 KASAN kernel. We (so
far) have _not_ seen this with an android 4.9 KASAN kernel (which has the
SOCK_RCU_FREE adjustments). There is no standalone duplication or PoC
_except_ via the fuzzer. The rest of the statements stands based on this
tidbit (statements of general good code hygiene, not 100% sure SOCK_RCU_FREE
usage is completely covered, KISS solution etc).
To be honest, yes, this may be a layer in the onion (swat this NULL check
does not by itself solve the _problem_), I'd prefer kernel continuing on in
a rational manner rather than panic ... and I have a gut feeling this could
be a gratuitous NULL check if all the bugs in the network layer have been
solved <that may be sarcasm, I can not tell>. Programming to solve a problem
with one's gut is not a good practice, but hygiene is. This is 10
characters, and an estimated 1.2ns of added hygiene.
No, I do not think this is limited to setsockopt, but would be willing to
believe a multithreaded attack of any socket functions or ioctl would drop
down to the check with sock_has_perm at possibly the wrong time in socket
teardown.

I'm not necessarily opposed to adding additional safety checks, if
warranted, but I am opposed to adding a single check and declaring
mission accomplished when there is a suspicion that additional checks
may be needed.

Perhaps in this particular case it really is only setsockopt(), but
from what I can tell from your comments and the SOCK_RCU_FREE commit
message it would appear that there is a race condition here between a
socket's lifetime and its visibility to userspace. Assuming this is
the core problem you are trying to workaround with this patch, I
suspect that more than just the SELinux/LSM hook for setsockopt() is
affected; before merging this patch I would like to see a better
investigation into all the socket related SELinux/LSM hooks to see if
they suffer from the same problem.

For example, if we stick with the setsockopt() syscall and the
SELinux/LSM hook we can see that there are two functions which access
the socket struct: sock_has_perm() and
selinux_netlbl_socket_setsockopt(). While you did add the safety
check to sock_has_perm() you neglected to add a check to
selinux_netlbl_socket_setsockopt(), I'm guessing because the Android
kernels probably do not enable CONFIG_NETLABEL. Looking beyond
setsockopt() into other socket related syscalls I see a number of
hooks which should have similar protections.

When you see problems like this, please do the investigation to make
sure you are fixing everything like it and not just the one instance
that blew up.

--
paul moore
www.paul-moore.com

Casey Schaufler

2018-01-19 17:37:43 UTC

Permalink

Post by Paul Moore

Post by Mark Salyzyn

Post by Paul Moore

general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
. . .
[<ffffffff81b6a19d>] selinux_socket_setsockopt+0x4d/0x80
security/selinux/hooks.c:4338
[<ffffffff81b4873d>] security_socket_setsockopt+0x7d/0xb0
security/security.c:1257
[<ffffffff82df1ac8>] SYSC_setsockopt net/socket.c:1757 [inline]
[<ffffffff82df1ac8>] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[<ffffffff83776499>] entry_SYSCALL_64_fastpath+0x16/0x92
Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
00 41 8b 75 10 31
RIP [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0
security/selinux/hooks.c:4069
RSP <ffff8800b5957ce0>
---[ end trace 7b5aaf788fef6174 ]---
In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
flag") and all the associated infrastructure changes to take advantage
of a RCU grace period before freeing, there is a heightened
possibility that a security check is performed while an ill-timed
setsockopt call races in from user space. It then is prudent to null
check sk_security, and if the case, reject the permissions.
This adjustment is orthogonal to infrastructure improvements that may
nullify the needed check, but should be added as good code hygiene.

I'm skeptical that this is the full solution for systems that lack the
SOCK_RCU_FREE protection. Is this really limited to just
setsockopt()?

Maybe overstepped in my analysis and assumptions?
This is a result of a fuzzer hitting an android 4.4 KASAN kernel. We (so
far) have _not_ seen this with an android 4.9 KASAN kernel (which has the
SOCK_RCU_FREE adjustments). There is no standalone duplication or PoC
_except_ via the fuzzer. The rest of the statements stands based on this
tidbit (statements of general good code hygiene, not 100% sure SOCK_RCU_FREE
usage is completely covered, KISS solution etc).
To be honest, yes, this may be a layer in the onion (swat this NULL check
does not by itself solve the _problem_), I'd prefer kernel continuing on in
a rational manner rather than panic ... and I have a gut feeling this could
be a gratuitous NULL check if all the bugs in the network layer have been
solved <that may be sarcasm, I can not tell>. Programming to solve a problem
with one's gut is not a good practice, but hygiene is. This is 10
characters, and an estimated 1.2ns of added hygiene.
No, I do not think this is limited to setsockopt, but would be willing to
believe a multithreaded attack of any socket functions or ioctl would drop
down to the check with sock_has_perm at possibly the wrong time in socket
teardown.

There have been a few cases where Smack has been affected
in similar ways outside of setsockopt(). If there's a problem
here there's likely to be a problem in Smack that presents
itself in a very different way.

Post by Paul Moore
For example, if we stick with the setsockopt() syscall and the
SELinux/LSM hook we can see that there are two functions which access
the socket struct: sock_has_perm() and
selinux_netlbl_socket_setsockopt(). While you did add the safety
check to sock_has_perm() you neglected to add a check to
selinux_netlbl_socket_setsockopt(), I'm guessing because the Android
kernels probably do not enable CONFIG_NETLABEL. Looking beyond
setsockopt() into other socket related syscalls I see a number of
hooks which should have similar protections.
When you see problems like this, please do the investigation to make
sure you are fixing everything like it and not just the one instance
that blew up.

Mark Salyzyn

2018-01-19 17:46:55 UTC

Permalink

Post by Paul Moore

Post by Mark Salyzyn

Post by Paul Moore

general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
. . .
[<ffffffff81b6a19d>] selinux_socket_setsockopt+0x4d/0x80
security/selinux/hooks.c:4338
[<ffffffff81b4873d>] security_socket_setsockopt+0x7d/0xb0
security/security.c:1257
[<ffffffff82df1ac8>] SYSC_setsockopt net/socket.c:1757 [inline]
[<ffffffff82df1ac8>] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[<ffffffff83776499>] entry_SYSCALL_64_fastpath+0x16/0x92
Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
00 41 8b 75 10 31
RIP [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0
security/selinux/hooks.c:4069
RSP <ffff8800b5957ce0>
---[ end trace 7b5aaf788fef6174 ]---
In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
flag") and all the associated infrastructure changes to take advantage
of a RCU grace period before freeing, there is a heightened
possibility that a security check is performed while an ill-timed
setsockopt call races in from user space. It then is prudent to null
check sk_security, and if the case, reject the permissions.
This adjustment is orthogonal to infrastructure improvements that may
nullify the needed check, but should be added as good code hygiene.

I'm skeptical that this is the full solution for systems that lack the
SOCK_RCU_FREE protection. Is this really limited to just
setsockopt()?

Maybe overstepped in my analysis and assumptions?
This is a result of a fuzzer hitting an android 4.4 KASAN kernel. We (so
far) have _not_ seen this with an android 4.9 KASAN kernel (which has the
SOCK_RCU_FREE adjustments). There is no standalone duplication or PoC
_except_ via the fuzzer. The rest of the statements stands based on this
tidbit (statements of general good code hygiene, not 100% sure SOCK_RCU_FREE
usage is completely covered, KISS solution etc).
To be honest, yes, this may be a layer in the onion (swat this NULL check
does not by itself solve the _problem_), I'd prefer kernel continuing on in
a rational manner rather than panic ... and I have a gut feeling this could
be a gratuitous NULL check if all the bugs in the network layer have been
solved <that may be sarcasm, I can not tell>. Programming to solve a problem
with one's gut is not a good practice, but hygiene is. This is 10
characters, and an estimated 1.2ns of added hygiene.
No, I do not think this is limited to setsockopt, but would be willing to
believe a multithreaded attack of any socket functions or ioctl would drop
down to the check with sock_has_perm at possibly the wrong time in socket
teardown.

I will need to fix my comments to be clearer ... (besides, I got the
error return wrong, so I will have to respin it anyways).

In later kernels SOCK_RCU_FREE _appears_ to fix the race condition. In
earlier kernels there is _no_ SOCK_RCU_FREE infrastructure, and thus the
race condition exists. That race conditions _should_ have been solved in
ToT. I have evaluated that porting all the SOCK_RCU_FREE refactoring may
be too risky an endeavor for stable kernels though (maybe I am wrong
about that? The author of those fixes ***@google.com has been
notified, have yet to receive a response).

Because I can not confirm that SOCK_RCU_FREE solves the problem, I am
making a case for this to land in 4.9+ to ToT kernels as good hygiene.
But maybe this should only go in 4.4- stable ...

-- Mark

Stephen Smalley

2018-01-19 17:19:46 UTC

Permalink

general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b
#28
task: ffff8801d1095f00 task.stack: ffff8800b5950000
RIP: 0010:[<ffffffff81b69b7e>] [<ffffffff81b69b7e>]
sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
RSP: 0018:ffff8800b5957ce0 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 1ffff10016b2af9f RCX: ffffffff81b69b51
RDX: 0000000000000002 RSI: 0000000000000000 RDI: 0000000000000010
RBP: ffff8800b5957de0 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: 1ffff10016b2af68 R12: ffff8800b5957db8
R13: 0000000000000000 R14: ffff8800b7259f40 R15: 00000000000000d7
FS: 00007f72f5ae2700(0000) GS:ffff8801db300000(0000)
knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000a2fa38 CR3: 00000001d7980000 CR4: 0000000000160670
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30 0000000041b58ab3
ffffffff83fc82f2 ffffffff81b69980 0000000000000246 ffff8801d1096770
ffff8801d3165668 ffffffff8157844b ffff8801d1095f00
ffff880000000001
[<ffffffff81b6a19d>] selinux_socket_setsockopt+0x4d/0x80
security/selinux/hooks.c:4338
[<ffffffff81b4873d>] security_socket_setsockopt+0x7d/0xb0
security/security.c:1257
[<ffffffff82df1ac8>] SYSC_setsockopt net/socket.c:1757 [inline]
[<ffffffff82df1ac8>] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[<ffffffff83776499>] entry_SYSCALL_64_fastpath+0x16/0x92
Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
00 41 8b 75 10 31
RIP [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0
security/selinux/hooks.c:4069
RSP <ffff8800b5957ce0>
---[ end trace 7b5aaf788fef6174 ]---
In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
flag") and all the associated infrastructure changes to take
advantage
of a RCU grace period before freeing, there is a heightened
possibility that a security check is performed while an ill-timed
setsockopt call races in from user space. It then is prudent to null
check sk_security, and if the case, reject the permissions.
This adjustment is orthogonal to infrastructure improvements that may
nullify the needed check, but should be added as good code hygiene.
---
This patch should be applied to all stable trees (author wants
minimum of 3.18, 4.4, 4.9 and 4.14)
security/selinux/hooks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..95d7c8143373 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4342,7 +4342,7 @@ static int sock_has_perm(struct sock *sk, u32
perms)
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
- if (sksec->sid == SECINITSID_KERNEL)
+ if (!sksec || sksec->sid == SECINITSID_KERNEL)
return 0;

The patch description says "null check the sk_security, and if the
case, reject the permissions." The patch code instead has it return
0/success, i.e. permission granted. Which one is correct? If we
return -EACCES, then we might break userspace; if we return 0, we might
be allowing an operation that should have been denied. Both seem like
losing propositions.

Could we instead have selinux_sk_free_security() defer freeing of the
sock security blob to a call_rcu(), like we did for
inode_free_security, or change the caller of it to not free it until
the sock is truly freed?

ad.type = LSM_AUDIT_DATA_NET;

Stephen Smalley

2018-01-19 17:41:12 UTC

Permalink

Post by Stephen Smalley

general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b
#28
task: ffff8801d1095f00 task.stack: ffff8800b5950000
RIP: 0010:[<ffffffff81b69b7e>] [<ffffffff81b69b7e>]
sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
RSP: 0018:ffff8800b5957ce0 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 1ffff10016b2af9f RCX: ffffffff81b69b51
RDX: 0000000000000002 RSI: 0000000000000000 RDI: 0000000000000010
RBP: ffff8800b5957de0 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: 1ffff10016b2af68 R12: ffff8800b5957db8
R13: 0000000000000000 R14: ffff8800b7259f40 R15: 00000000000000d7
FS: 00007f72f5ae2700(0000) GS:ffff8801db300000(0000)
knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000a2fa38 CR3: 00000001d7980000 CR4: 0000000000160670
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30
0000000041b58ab3
ffffffff83fc82f2 ffffffff81b69980 0000000000000246
ffff8801d1096770
ffff8801d3165668 ffffffff8157844b ffff8801d1095f00
ffff880000000001
[<ffffffff81b6a19d>] selinux_socket_setsockopt+0x4d/0x80
security/selinux/hooks.c:4338
[<ffffffff81b4873d>] security_socket_setsockopt+0x7d/0xb0
security/security.c:1257
[<ffffffff82df1ac8>] SYSC_setsockopt net/socket.c:1757 [inline]
[<ffffffff82df1ac8>] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[<ffffffff83776499>] entry_SYSCALL_64_fastpath+0x16/0x92
Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
00 41 8b 75 10 31
RIP [<ffffffff81b69b7e>] sock_has_perm+0x1fe/0x3e0
security/selinux/hooks.c:4069
RSP <ffff8800b5957ce0>
---[ end trace 7b5aaf788fef6174 ]---
In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
flag") and all the associated infrastructure changes to take
advantage
of a RCU grace period before freeing, there is a heightened
possibility that a security check is performed while an ill-timed
setsockopt call races in from user space. It then is prudent to null
check sk_security, and if the case, reject the permissions.
This adjustment is orthogonal to infrastructure improvements that may
nullify the needed check, but should be added as good code hygiene.
---
This patch should be applied to all stable trees (author wants
minimum of 3.18, 4.4, 4.9 and 4.14)
security/selinux/hooks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..95d7c8143373 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4342,7 +4342,7 @@ static int sock_has_perm(struct sock *sk, u32
perms)
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
- if (sksec->sid == SECINITSID_KERNEL)
+ if (!sksec || sksec->sid == SECINITSID_KERNEL)
return 0;

The patch description says "null check the sk_security, and if the
case, reject the permissions." The patch code instead has it return
0/success, i.e. permission granted. Which one is correct? If we
return -EACCES, then we might break userspace; if we return 0, we might
be allowing an operation that should have been denied. Both seem like
losing propositions.
Could we instead have selinux_sk_free_security() defer freeing of the
sock security blob to a call_rcu(), like we did for
inode_free_security, or change the caller of it to not free it until
the sock is truly freed?

On second thought, I don't quite understand the problem you are trying
to solve. security_sk_free() is only called just prior to freeing the
sock by sk_prot_alloc() or sk_prot_free(). So if sk_security is NULL
in sock_has_perm(), then the sock is itself about to be or already
freed, and it isn't the sk_security that we ought to be worried about -
it is the sock itself that we shouldn't be using. Or am I missing
something?

If we can't safely dereference the sock in these hooks, then that seems
to point back to the approach used in my original code, where in
ancient history I had sock_has_perm() take the socket and use its inode
i_security field instead of the sock. commit
253bfae6e0ad97554799affa0266052968a45808 switched them to use the sock
instead.

Mark Salyzyn

2018-01-30 19:00:04 UTC

Permalink

Post by Stephen Smalley
If we can't safely dereference the sock in these hooks, then that seems
to point back to the approach used in my original code, where in
ancient history I had sock_has_perm() take the socket and use its inode
i_security field instead of the sock. commit
253bfae6e0ad97554799affa0266052968a45808 switched them to use the sock
instead.

Because of the nature of this problem (hard to duplicate, no clear
path), I am understandably not comfortable reverting and submitting for
testing in order to prove this point. It is disruptive because it
changes several subroutine call signatures.

AFAIK this looks like a user request racing in without reference
counting or RCU grace period in the callers (could be viewed as not an
issue with security code). Effectively fixed in 4.9-stable, but broken
in 4.4-stable.

hygiene, KISS and small, is all I do feel comfortable to submit to
4.4-stable without pulling in all the infrastructure improvements.

-- Mark

---
security/selinux/hooks.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 34427384605d..be68992a28cb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4066,6 +4066,8 @@ static int sock_has_perm(struct task_struct *task,
struct sock *sk, u32 perms)
     struct lsm_network_audit net = {0,};
     u32 tsid = task_sid(task);

+    if (!sksec)
+        return -EFAULT;
     if (sksec->sid == SECINITSID_KERNEL)
         return 0;

Greg KH

2018-01-30 22:46:14 UTC

Permalink

Because of the nature of this problem (hard to duplicate, no clear path), I
am understandably not comfortable reverting and submitting for testing in
order to prove this point. It is disruptive because it changes several
subroutine call signatures.
AFAIK this looks like a user request racing in without reference counting or
RCU grace period in the callers (could be viewed as not an issue with
security code). Effectively fixed in 4.9-stable, but broken in 4.4-stable.
hygiene, KISS and small, is all I do feel comfortable to submit to
4.4-stable without pulling in all the infrastructure improvements.
-- Mark
---
security/selinux/hooks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 34427384605d..be68992a28cb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4066,6 +4066,8 @@ static int sock_has_perm(struct task_struct *task,
struct sock *sk, u32 perms)
     struct lsm_network_audit net = {0,};
     u32 tsid = task_sid(task);
+    if (!sksec)
+        return -EFAULT;
     if (sksec->sid == SECINITSID_KERNEL)
         return 0;

This looks sane to me as a simple 4.4-only fix. If the SELinux
maintainer acks it, I can easily queue this up.

thanks,

greg k-h

Paul Moore

2018-01-31 09:06:37 UTC

Permalink

Post by Greg KH

Because of the nature of this problem (hard to duplicate, no clear path), I
am understandably not comfortable reverting and submitting for testing in
order to prove this point. It is disruptive because it changes several
subroutine call signatures.
AFAIK this looks like a user request racing in without reference counting or
RCU grace period in the callers (could be viewed as not an issue with
security code). Effectively fixed in 4.9-stable, but broken in 4.4-stable.
hygiene, KISS and small, is all I do feel comfortable to submit to
4.4-stable without pulling in all the infrastructure improvements.
-- Mark
---
security/selinux/hooks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 34427384605d..be68992a28cb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4066,6 +4066,8 @@ static int sock_has_perm(struct task_struct *task,
struct sock *sk, u32 perms)
struct lsm_network_audit net = {0,};
u32 tsid = task_sid(task);
+ if (!sksec)
+ return -EFAULT;
if (sksec->sid == SECINITSID_KERNEL)
return 0;

This looks sane to me as a simple 4.4-only fix. If the SELinux
maintainer acks it, I can easily queue this up.

This revision addresses my concerns with Mark's previous patch.

Acked-by: Paul Moore <***@paul-moore.com>

--
paul moore
www.paul-moore.com

Greg KH

2018-02-01 08:18:11 UTC

Permalink

Post by Paul Moore

Post by Greg KH

Because of the nature of this problem (hard to duplicate, no clear path), I
am understandably not comfortable reverting and submitting for testing in
order to prove this point. It is disruptive because it changes several
subroutine call signatures.
AFAIK this looks like a user request racing in without reference counting or
RCU grace period in the callers (could be viewed as not an issue with
security code). Effectively fixed in 4.9-stable, but broken in 4.4-stable.
hygiene, KISS and small, is all I do feel comfortable to submit to
4.4-stable without pulling in all the infrastructure improvements.
-- Mark
---
security/selinux/hooks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 34427384605d..be68992a28cb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4066,6 +4066,8 @@ static int sock_has_perm(struct task_struct *task,
struct sock *sk, u32 perms)
struct lsm_network_audit net = {0,};
u32 tsid = task_sid(task);
+ if (!sksec)
+ return -EFAULT;
if (sksec->sid == SECINITSID_KERNEL)
return 0;

This looks sane to me as a simple 4.4-only fix. If the SELinux
maintainer acks it, I can easily queue this up.

This revision addresses my concerns with Mark's previous patch.

Wonderful!

Mark, can you resend this in a format I can apply it in?

thanks,

greg k-h

Mark Salyzyn

2018-01-19 17:34:36 UTC

Permalink

Post by Stephen Smalley

general protection fault: 0000 [#1] PREEMPT SMP KASAN
. . .
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..95d7c8143373 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4342,7 +4342,7 @@ static int sock_has_perm(struct sock *sk, u32
perms)
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
- if (sksec->sid == SECINITSID_KERNEL)
+ if (!sksec || sksec->sid == SECINITSID_KERNEL)
return 0;

The patch description says "null check the sk_security, and if the
case, reject the permissions." The patch code instead has it return
0/success, i.e. permission granted. Which one is correct?

<oops> -EACCESS would be advised, yes. THANKS.

<please remove my mistake from my permanent record ;-} >

Post by Stephen Smalley
If we
return -EACCES, then we might break userspace; if we return 0, we might
be allowing an operation that should have been denied. Both seem like
losing propositions.

if the sk_security is NULL, it is in-effect a form of UAF, so kernel
_and_ user space is already 'sick'. I think it is a significantly larger
losing proposition to panic the kernel? Reporting -EACCESS (as was
proper) is a error propagation way to let user space deal with the
erroneous condition.

Post by Stephen Smalley
Could we instead have selinux_sk_free_security() defer freeing of the
sock security blob to a call_rcu(), like we did for
inode_free_security, or change the caller of it to not free it until
the sock is truly freed?

AFAIK the upper issue is the premature closing on an RCU protected
object, and the _right_ answer is that its call should have been
properly deferred to a synchronization or grace period. Having
sk_free_security be deferred by the grace period runs the risk that it
is in a race with the proper deletion of a languishing read object in an
RCU. It is a bug in the upper layers. My proposal in this KISS stability
patch is to make security deal with those bugs gracefully until all
those issues are fixed (in ToT).

-- Mark