[PATCH] general protection fault in sock_has_perm

[PATCH] general protection fault in sock_has_perm

[Mark Salyzyn]

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 ]---

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.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: James Morris <james.l.morris@oracle.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: selinux@tycho.nsa.gov
Cc: linux-security-module@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
---
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

Re: [PATCH] general protection fault in sock_has_perm

[Paul Moore]

On Thu, Jan 18, 2018 at 4:58 PM, Mark Salyzyn <salyzyn@android.com> wrote:
> 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 ]---
>
> 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()?

> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: selinux@tycho.nsa.gov
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
> 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

Re: [PATCH] general protection fault in sock_has_perm

[Greg KH]

On Thu, Jan 18, 2018 at 01:58:45PM -0800, Mark Salyzyn wrote:
> 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 ]---
> 
> 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.
> 
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: selinux@tycho.nsa.gov
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
> This patch should be applied to all stable trees (author wants
> minimum of 3.18, 4.4, 4.9 and 4.14)

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 <stable@vger.kernel.org> # 3.18+

thanks,

greg k-h

Re: [PATCH] general protection fault in sock_has_perm

[Mark Salyzyn]

On 01/18/2018 02:36 PM, Paul Moore wrote:
> On Thu, Jan 18, 2018 at 4:58 PM, Mark Salyzyn <salyzyn@android.com> wrote:
>> 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

Re: [PATCH] general protection fault in sock_has_perm

[Paul Moore]

On Fri, Jan 19, 2018 at 10:49 AM, Mark Salyzyn <salyzyn@android.com> wrote:
> On 01/18/2018 02:36 PM, Paul Moore wrote:
>> On Thu, Jan 18, 2018 at 4:58 PM, Mark Salyzyn <salyzyn@android.com> wrote:
>>> 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

Re: [PATCH] general protection fault in sock_has_perm

[Stephen Smalley]

On Thu, 2018-01-18 at 13:58 -0800, Mark Salyzyn wrote:
> 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 ]---
> 
> 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.
> 
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: selinux@tycho.nsa.gov
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
> 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;

Re: [PATCH] general protection fault in sock_has_perm

[Mark Salyzyn]

On 01/19/2018 09:19 AM, Stephen Smalley wrote:
> On Thu, 2018-01-18 at 13:58 -0800, Mark Salyzyn wrote:
>> 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 ;-} >
> 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.

>
> 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

Re: [PATCH] general protection fault in sock_has_perm

[Casey Schaufler]

On 1/19/2018 9:06 AM, Paul Moore wrote:
> On Fri, Jan 19, 2018 at 10:49 AM, Mark Salyzyn <salyzyn@android.com> wrote:
>> On 01/18/2018 02:36 PM, Paul Moore wrote:
>>> On Thu, Jan 18, 2018 at 4:58 PM, Mark Salyzyn <salyzyn@android.com> wrote:
>>>> 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.

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.

> 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.
>

Re: [PATCH] general protection fault in sock_has_perm

[Stephen Smalley]

On Fri, 2018-01-19 at 12:19 -0500, Stephen Smalley wrote:
> On Thu, 2018-01-18 at 13:58 -0800, Mark Salyzyn wrote:
> > 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 ]---
> > 
> > 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.
> > 
> > Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Stephen Smalley <sds@tycho.nsa.gov>
> > Cc: Eric Paris <eparis@parisplace.org>
> > Cc: James Morris <james.l.morris@oracle.com>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: selinux@tycho.nsa.gov
> > Cc: linux-security-module@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: stable@vger.kernel.org
> > ---
> > 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.

Re: [PATCH] general protection fault in sock_has_perm

[Mark Salyzyn]

On 01/19/2018 09:06 AM, Paul Moore wrote:
> On Fri, Jan 19, 2018 at 10:49 AM, Mark Salyzyn <salyzyn@android.com> wrote:
>> On 01/18/2018 02:36 PM, Paul Moore wrote:
>>> On Thu, Jan 18, 2018 at 4:58 PM, Mark Salyzyn <salyzyn@android.com> wrote:
>>>> 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.

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 edumazet@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

Re: [PATCH] general protection fault in sock_has_perm

[Mark Salyzyn]

On 01/19/2018 09:41 AM, Stephen Smalley wrote:
> 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;

Re: [PATCH] general protection fault in sock_has_perm

[Greg KH]

On Tue, Jan 30, 2018 at 11:00:04AM -0800, Mark Salyzyn wrote:
> On 01/19/2018 09:41 AM, Stephen Smalley wrote:
> > 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;
> 

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

Re: [PATCH] general protection fault in sock_has_perm

[Paul Moore]

On Tue, Jan 30, 2018 at 5:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Jan 30, 2018 at 11:00:04AM -0800, Mark Salyzyn wrote:
>> On 01/19/2018 09:41 AM, Stephen Smalley wrote:
>> > 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;
>>
>
> 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@paul-moore.com>

-- 
paul moore
www.paul-moore.com

Re: [PATCH] general protection fault in sock_has_perm

[Greg KH]

On Wed, Jan 31, 2018 at 04:06:37AM -0500, Paul Moore wrote:
> On Tue, Jan 30, 2018 at 5:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Jan 30, 2018 at 11:00:04AM -0800, Mark Salyzyn wrote:
> >> On 01/19/2018 09:41 AM, Stephen Smalley wrote:
> >> > 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;
> >>
> >
> > 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@paul-moore.com>

Wonderful!

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

thanks,

greg k-h