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.
Because of the nature of this problem, hard to duplicate, no clear path, this patch is a simplified band-aid for stable trees lacking the infrastructure for the series of commits leading up to providing a suitable RCU grace period. This adjustment is orthogonal to infrastructure improvements that may nullify the needed check, but could be added as good code hygiene in all trees.
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 Stack: ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30 0000000041b58ab3 ffffffff83fc82f2 ffffffff81b69980 0000000000000246 ffff8801d1096770 ffff8801d3165668 ffffffff8157844b ffff8801d1095f00 ffff880000000001 Call Trace: [<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 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org Signed-off-by: Greg KH gregkh@linuxfoundation.org Cc: Eric Dumazet edumazet@google.com Cc: Stephen Smalley sds@tycho.nsa.gov Cc: selinux@tycho.nsa.gov Cc: linux-security-module@vger.kernel.org Cc: Eric Paris eparis@parisplace.org Cc: Serge E. Hallyn serge@hallyn.com Cc: stable stable@vger.kernel.org # 3.18, 4.4 Cc: linux-kernel@vger.kernel.org --- v2: return -EFAULT for null sk_security instead of 0
security/selinux/hooks.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 8644d864e3c1..795efa71d656 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4342,6 +4342,8 @@ static int sock_has_perm(struct sock *sk, u32 perms) struct common_audit_data ad; struct lsm_network_audit net = {0,};
+ if (!sksec) + return -EFAULT; if (sksec->sid == SECINITSID_KERNEL) return 0;
On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn salyzyn@android.com wrote:
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.
Because of the nature of this problem, hard to duplicate, no clear path, this patch is a simplified band-aid for stable trees lacking the infrastructure for the series of commits leading up to providing a suitable RCU grace period. This adjustment is orthogonal to infrastructure improvements that may nullify the needed check, but could be added as good code hygiene in all trees.
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 Stack: ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30 0000000041b58ab3 ffffffff83fc82f2 ffffffff81b69980 0000000000000246 ffff8801d1096770 ffff8801d3165668 ffffffff8157844b ffff8801d1095f00 ffff880000000001 Call Trace: [<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 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org
No, in the previous thread I gave my ack, not my sign-off; please be more careful in the future. It may seem silly, especially in this particular case, but it is an important distinction when things like the DCO are concerned.
Anyway, here is my ack again.
Acked-by: Paul Moore paul@paul-moore.com
Signed-off-by: Greg KH gregkh@linuxfoundation.org Cc: Eric Dumazet edumazet@google.com Cc: Stephen Smalley sds@tycho.nsa.gov Cc: selinux@tycho.nsa.gov Cc: linux-security-module@vger.kernel.org Cc: Eric Paris eparis@parisplace.org Cc: Serge E. Hallyn serge@hallyn.com Cc: stable stable@vger.kernel.org # 3.18, 4.4 Cc: linux-kernel@vger.kernel.org
v2: return -EFAULT for null sk_security instead of 0
security/selinux/hooks.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 8644d864e3c1..795efa71d656 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4342,6 +4342,8 @@ static int sock_has_perm(struct sock *sk, u32 perms) struct common_audit_data ad; struct lsm_network_audit net = {0,};
if (!sksec)return -EFAULT; if (sksec->sid == SECINITSID_KERNEL) return 0;-- 2.16.0.rc1.238.g530d649a79-goog
On 02/01/2018 08:00 AM, Paul Moore wrote:
On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn salyzyn@android.com wrote:
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.
. . . ---[ end trace 7b5aaf788fef6174 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org
No, in the previous thread I gave my ack, not my sign-off; please be more careful in the future. It may seem silly, especially in this particular case, but it is an important distinction when things like the DCO are concerned.
Anyway, here is my ack again.
Acked-by: Paul Moore paul@paul-moore.com
Ok, both Greg KH and yours should be considered Acked-By. Been overstepping this boundary for _years_. AFAIK Signed-off-by is still pending from Stephen Smalley sds@tycho.nsa.gov before this can roll in.
Lesson lurned
-- Mark
On Thu, Feb 1, 2018 at 11:20 AM, Mark Salyzyn salyzyn@android.com wrote:
On 02/01/2018 08:00 AM, Paul Moore wrote:
On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn salyzyn@android.com wrote:
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.
. . . ---[ end trace 7b5aaf788fef6174 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org
No, in the previous thread I gave my ack, not my sign-off; please be more careful in the future. It may seem silly, especially in this particular case, but it is an important distinction when things like the DCO are concerned.
Anyway, here is my ack again.
Acked-by: Paul Moore paul@paul-moore.com
Ok, both Greg KH and yours should be considered Acked-By. Been overstepping this boundary for _years_.
One more note, which I didn't realize until I hit reply and the email bounced: you used a @linuxfoundation.org email address for me which is clearly not right. I'm sure it's just a typo, but it's another thing that needs to be corrected.
AFAIK Signed-off-by is still pending from Stephen Smalley sds@tycho.nsa.gov before this can roll in.
Obviously the more acks the better, but you've got mine which should be sufficient in this case. The MAINTAINER file currently lists three people for SELinux: Stephen, Eric, and myself. I'm responsible for the traditional maintainer tasks: tree management, PRs to Linus, patch review, emptying the waste bin at the end of the week, etc. Stephen maintains the deep historical knowledge and understanding that comes with developing the technology/project from it's inception many, many years ago; no matter how well I may understand SELinux, Stephen will always have me beat. Eric is basically my predecessor, having ventured off to the brave new world of containers and Kubernetes; he is listed out of respect for his contributions and also to safeguard us against the all important "bus factor", while he is not as active as he once was, he still holds a wealth of SELinux knowledge.
On Thu, 2018-02-01 at 08:20 -0800, Mark Salyzyn wrote:
On 02/01/2018 08:00 AM, Paul Moore wrote:
On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn salyzyn@android.com wrote:
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.
. . . ---[ end trace 7b5aaf788fef6174 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org
No, in the previous thread I gave my ack, not my sign-off; please be more careful in the future. It may seem silly, especially in this particular case, but it is an important distinction when things like the DCO are concerned.
Anyway, here is my ack again.
Acked-by: Paul Moore paul@paul-moore.com
Ok, both Greg KH and yours should be considered Acked-By. Been overstepping this boundary for _years_. AFAIK Signed-off-by is still pending from Stephen Smalley sds@tycho.nsa.gov before this can roll in.
Lesson lurned
No, Paul's Acked-by is sufficient, and at most, I would only add another Acked-by or Reviewed-by, not a Signed-off-by. Signed-off-by is only needed when one had something to do with the writing of the patch or was in the path by which it was merged.
I don't object to this patch but I have a hard time adding another ack because I don't truly understand the root cause or how this fixes it. Let's say sk_prot_free() calls security_sk_free() calls selinux_sk_free_security() which sets sk->sk_security to NULL, and then we proceed to free the sksec and then sk_prot_free() frees the sk itself. Now another sock is allocated (or perhaps a different object altogether), reuses that memory, and whatever sk->sk_security happens to contain is set to non-NULL. We'll just blithely proceed past your check and who knows what will happen from that point onward.
On 02/01/2018 09:02 AM, Stephen Smalley wrote:
On Thu, 2018-02-01 at 08:20 -0800, Mark Salyzyn wrote:
On 02/01/2018 08:00 AM, Paul Moore wrote:
On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn salyzyn@android.com wrote:
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.
. . . ---[ end trace 7b5aaf788fef6174 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org
No, in the previous thread I gave my ack, not my sign-off; please be more careful in the future. It may seem silly, especially in this particular case, but it is an important distinction when things like the DCO are concerned.
Anyway, here is my ack again.
Acked-by: Paul Moore paul@paul-moore.com
Ok, both Greg KH and yours should be considered Acked-By. Been overstepping this boundary for _years_. AFAIK Signed-off-by is still pending from Stephen Smalley sds@tycho.nsa.gov before this can roll in.
Lesson lurned
No, Paul's Acked-by is sufficient, and at most, I would only add another Acked-by or Reviewed-by, not a Signed-off-by. Signed-off-by is only needed when one had something to do with the writing of the patch or was in the path by which it was merged.
I don't object to this patch but I have a hard time adding another ack because I don't truly understand the root cause or how this fixes it. Let's say sk_prot_free() calls security_sk_free() calls selinux_sk_free_security() which sets sk->sk_security to NULL, and then we proceed to free the sksec and then sk_prot_free() frees the sk itself. Now another sock is allocated (or perhaps a different object altogether), reuses that memory, and whatever sk->sk_security happens to contain is set to non-NULL. We'll just blithely proceed past your check and who knows what will happen from that point onward.
The way I read this is this is part of an RCU operation. Multiple readers are holding on to the object, but as soon as a new writer comes in it _immediately_ frees the sk_security of the 'old' reader copies in order to make the 'new' writer copy. Any pending readers continue operations until they get tripped on the too aggressively released NULL sk_security reference.
Commits came in between 4.4 and 4.9 (edumazet@google.com) to restructure and fix this and add the appropriate RCU grace period to the 'old' reader copies for the sk_security resource so that it would be freed after all the readers had exited. Problem goes away.
My proposal will break any 'old' readers by blocking their access during the transition rather than panic the kernel. New readers coming in after the writer will progress fine.
This is not a 'bug' in the security layer, this is a bandaid to the security layer regarding the bad behavior of the callers.
I have not analyzed the code enough to 100% prove my assertion above, in part because I can not duplicate the problem w/o kasan+fuzzing, so still treat this as a hunch.
-- Mark
On Thu, Feb 01, 2018 at 08:20:13AM -0800, Mark Salyzyn wrote:
On 02/01/2018 08:00 AM, Paul Moore wrote:
On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn salyzyn@android.com wrote:
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.
. . . ---[ end trace 7b5aaf788fef6174 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org
No, in the previous thread I gave my ack, not my sign-off; please be more careful in the future. It may seem silly, especially in this particular case, but it is an important distinction when things like the DCO are concerned.
Anyway, here is my ack again.
Acked-by: Paul Moore paul@paul-moore.com
Ok, both Greg KH and yours should be considered Acked-By. Been overstepping this boundary for _years_. AFAIK Signed-off-by is still pending from Stephen Smalley sds@tycho.nsa.gov before this can roll in.
An ack is all I need here, or I can just rely on Paul's :) I'll edit up Paul's when I apply this.
thanks,
greg k-h
On Thu, Feb 01, 2018 at 07:37:04AM -0800, Mark Salyzyn wrote:
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.
Because of the nature of this problem, hard to duplicate, no clear path, this patch is a simplified band-aid for stable trees lacking the infrastructure for the series of commits leading up to providing a suitable RCU grace period. This adjustment is orthogonal to infrastructure improvements that may nullify the needed check, but could be added as good code hygiene in all trees.
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 Stack: ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30 0000000041b58ab3 ffffffff83fc82f2 ffffffff81b69980 0000000000000246 ffff8801d1096770 ffff8801d3165668 ffffffff8157844b ffff8801d1095f00 ffff880000000001 Call Trace: [<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 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Signed-off-by: Paul Moore paul@linuxfoundation.org Signed-off-by: Greg KH gregkh@linuxfoundation.org Cc: Eric Dumazet edumazet@google.com Cc: Stephen Smalley sds@tycho.nsa.gov Cc: selinux@tycho.nsa.gov Cc: linux-security-module@vger.kernel.org Cc: Eric Paris eparis@parisplace.org Cc: Serge E. Hallyn serge@hallyn.com Cc: stable stable@vger.kernel.org # 3.18, 4.4 Cc: linux-kernel@vger.kernel.org
v2: return -EFAULT for null sk_security instead of 0
Now queued up, thanks.
greg k-h
This is a note to let you know that I've just added the patch titled
selinux: general protection fault in sock_has_perm
to the 3.18-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is: selinux-general-protection-fault-in-sock_has_perm.patch and it can be found in the queue-3.18 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let stable@vger.kernel.org know about it.
From salyzyn@android.com Fri Feb 2 11:25:36 2018
From: Mark Salyzyn salyzyn@android.com Date: Thu, 1 Feb 2018 07:37:04 -0800 Subject: selinux: general protection fault in sock_has_perm To: linux-kernel@vger.kernel.org Cc: Mark Salyzyn salyzyn@android.com, Paul Moore paul@linuxfoundation.org, Greg KH gregkh@linuxfoundation.org, Eric Dumazet edumazet@google.com, Stephen Smalley sds@tycho.nsa.gov, selinux@tycho.nsa.gov, linux-security-module@vger.kernel.org, Eric Paris eparis@parisplace.org, "Serge E . Hallyn" serge@hallyn.com, stable stable@vger.kernel.org, Paul Moore paul@paul-moore.com, James Morris jmorris@namei.org Message-ID: 20180201153708.63506-1-salyzyn@android.com
From: Mark Salyzyn salyzyn@android.com
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.
Because of the nature of this problem, hard to duplicate, no clear path, this patch is a simplified band-aid for stable trees lacking the infrastructure for the series of commits leading up to providing a suitable RCU grace period. This adjustment is orthogonal to infrastructure improvements that may nullify the needed check, but could be added as good code hygiene in all trees.
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 Stack: ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30 0000000041b58ab3 ffffffff83fc82f2 ffffffff81b69980 0000000000000246 ffff8801d1096770 ffff8801d3165668 ffffffff8157844b ffff8801d1095f00 ffff880000000001 Call Trace: [<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 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Acked-by: Paul Moore paul@paul-moore.com Cc: Eric Dumazet edumazet@google.com Cc: Stephen Smalley sds@tycho.nsa.gov Cc: selinux@tycho.nsa.gov Cc: linux-security-module@vger.kernel.org Cc: Eric Paris eparis@parisplace.org Cc: Serge E. Hallyn serge@hallyn.com Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
--- security/selinux/hooks.c | 2 ++ 1 file changed, 2 insertions(+)
--- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3969,6 +3969,8 @@ static int sock_has_perm(struct task_str struct lsm_network_audit net = {0,}; u32 tsid = task_sid(task);
+ if (!sksec) + return -EFAULT; if (sksec->sid == SECINITSID_KERNEL) return 0;
Patches currently in stable-queue which might be from salyzyn@android.com are
queue-3.18/selinux-general-protection-fault-in-sock_has_perm.patch
This is a note to let you know that I've just added the patch titled
selinux: general protection fault in sock_has_perm
to the 4.4-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
The filename of the patch is: selinux-general-protection-fault-in-sock_has_perm.patch and it can be found in the queue-4.4 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let stable@vger.kernel.org know about it.
From salyzyn@android.com Fri Feb 2 11:25:36 2018
From: Mark Salyzyn salyzyn@android.com Date: Thu, 1 Feb 2018 07:37:04 -0800 Subject: selinux: general protection fault in sock_has_perm To: linux-kernel@vger.kernel.org Cc: Mark Salyzyn salyzyn@android.com, Paul Moore paul@linuxfoundation.org, Greg KH gregkh@linuxfoundation.org, Eric Dumazet edumazet@google.com, Stephen Smalley sds@tycho.nsa.gov, selinux@tycho.nsa.gov, linux-security-module@vger.kernel.org, Eric Paris eparis@parisplace.org, "Serge E . Hallyn" serge@hallyn.com, stable stable@vger.kernel.org, Paul Moore paul@paul-moore.com, James Morris jmorris@namei.org Message-ID: 20180201153708.63506-1-salyzyn@android.com
From: Mark Salyzyn salyzyn@android.com
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.
Because of the nature of this problem, hard to duplicate, no clear path, this patch is a simplified band-aid for stable trees lacking the infrastructure for the series of commits leading up to providing a suitable RCU grace period. This adjustment is orthogonal to infrastructure improvements that may nullify the needed check, but could be added as good code hygiene in all trees.
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 Stack: ffffffff81b69a1f ffff8800b5957d58 00008000b5957d30 0000000041b58ab3 ffffffff83fc82f2 ffffffff81b69980 0000000000000246 ffff8801d1096770 ffff8801d3165668 ffffffff8157844b ffff8801d1095f00 ffff880000000001 Call Trace: [<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 ]---
Signed-off-by: Mark Salyzyn salyzyn@android.com Acked-by: Paul Moore paul@paul-moore.com Cc: Eric Dumazet edumazet@google.com Cc: Stephen Smalley sds@tycho.nsa.gov Cc: selinux@tycho.nsa.gov Cc: linux-security-module@vger.kernel.org Cc: Eric Paris eparis@parisplace.org Cc: Serge E. Hallyn serge@hallyn.com Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
--- security/selinux/hooks.c | 2 ++ 1 file changed, 2 insertions(+)
--- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4032,6 +4032,8 @@ static int sock_has_perm(struct task_str struct lsm_network_audit net = {0,}; u32 tsid = task_sid(task);
+ if (!sksec) + return -EFAULT; if (sksec->sid == SECINITSID_KERNEL) return 0;
Patches currently in stable-queue which might be from salyzyn@android.com are
queue-4.4/selinux-general-protection-fault-in-sock_has_perm.patch
linux-stable-mirror@lists.linaro.org
-
Greg KH -
gregkh＠linuxfoundation.org
-
Mark Salyzyn -
Paul Moore -
Stephen Smalley