Re: [PATCH v2] general protection fault in sock_has_perm

[Paul Moore]

On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn <salyzyn@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxxxxxxx>

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

> Signed-off-by: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> Cc: Stephen Smalley <sds@xxxxxxxxxxxxx>
> Cc: selinux@xxxxxxxxxxxxx
> Cc: linux-security-module@xxxxxxxxxxxxxxx
> Cc: Eric Paris <eparis@xxxxxxxxxxxxxx>
> Cc: Serge E. Hallyn <serge@xxxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxxxxxxx> # 3.18, 4.4
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> 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

-- 
paul moore
www.paul-moore.com