Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-PHdr: =?us-ascii?q?9a23=3AG4VYlRdsVhJPWKLGo364MJNZlGMj4u6mDksu8pMi?=
 =?us-ascii?q?zoh2WeGdxc24Yx2N2/xhgRfzUJnB7Loc0qyK6/qmATJLuM7Z+Fk5M7V0Hycfjs?=
 =?us-ascii?q?sXmwFySOWkMmbcaMDQUiohAc5ZX0Vk9XzoeWJcGcL5ekGA6ibqtW1aFRrwLxd6?=
 =?us-ascii?q?KfroEYDOkcu3y/qy+5rOaAlUmTaxe7x/IAmooQnLqsUbgIRuJrstxhfVv3BFZ/?=
 =?us-ascii?q?lYyWR0KFyJgh3y/N2w/Jlt8yRRv/Iu6ctNWrjkcqo7ULJVEi0oP3g668P3uxbD?=
 =?us-ascii?q?SxCP5mYHXWUNjhVIGQnF4wrkUZr3ryD3q/By2CiePc3xULA0RTGv5LplRRP0lC?=
 =?us-ascii?q?sKMSMy/XrJgcJskq1UvBOhpwR+w4HKZoGVKOF+db7Zcd8DWGZNQtpdWylHD4yy?=
 =?us-ascii?q?dYsPC/cKM/heoYfzulACqQKyCRewCO/qzDJDm3340rAg0+k5Eg/IwQwuEcwAvn?=
 =?us-ascii?q?vWotX1M7sdX+e6w6fH1jjDc/Fb1C3h5IXSbhwso/eBVq9wf8rLzkkvEhvIgEiM?=
 =?us-ascii?q?qYP7JzOV1voCs26G5OR9UOKgkWonqwVvrTmv28whjZLJiZ8Oyl3f6SV4wJo6Jd?=
 =?us-ascii?q?2/SEJhZ96kC4FfuzuVN4txXMMvWmdlszs5xL0eoZO3YScHxZs9yxPfdvCLaZaE?=
 =?us-ascii?q?7x39WOqLPDt1gm9udqiliBao60egz/XxVsyz0FlXsCVIisLMtnUR1xzL7ciHV+?=
 =?us-ascii?q?d98l+h2TmR0wDT7flJIVwumqrBKp4h36UwmoAPsUXDAiD2mEL2gLWQdko44ein?=
 =?us-ascii?q?9/7rYrDnpp+YL4N0iwf+PboymsGnHOg1PQcDU3Kb9OihzrHv40L0TKtQgvEriq?=
 =?us-ascii?q?XZtYrVJcUfpq63GQ9V1YMj5g6kDzi7y9QVhmUHLVJZdxKHiIjlIVfOIOviAvul?=
 =?us-ascii?q?jFSslylry+jcPrL9GpXNMmTDkLD5cLZm905T0hE8zdRB6J9PFLEBL+z8WlXruN?=
 =?us-ascii?q?zbEBA5KQq0zPjjCNln0YMeQ22PCLeDMKzOqV+I+v4vI+6UaY8RuTb9LeUl5vH3?=
 =?us-ascii?q?gX8ih1ASYbSp3YEWaHCkHvVqOkCZYX3xjdccFWcFoBEzTPLliFKcSz5ffXWyUL?=
 =?us-ascii?q?wm5jE9Fo2mCZ3PRoe3gLyOxC27BIFZZnhaClCQFnflb56EVOkWaCKdPMBsiTwE?=
 =?us-ascii?q?WqKlS48l1RCushX2xKZgLurR4icYr47s1MBp5+3PkhE/7T50AN6Y026TVGF4hG?=
 =?us-ascii?q?cISyUz3KB4u0x90FaD0bNjjvxfD9xc/e9GUgMkOpLG0+N6DNXyUBrbftiVUFam?=
 =?us-ascii?q?XsmmATYpQ90v298BeVx9G9S5jh3YxyqlGaUVl72QBJws9qLTxWT+KNhnx3bBzq?=
 =?us-ascii?q?khgEEsQtFTOm2+mq5/6w/TCpbRk0qDiqaqcb8R3DbX+2eeyWqCpURYUAl3UaXf?=
 =?us-ascii?q?Q38TfFfZrdP85knaVb+hFawnMhddyc6FMqZKbtzpjVNbRPbsIdjeYHy+m322BR?=
 =?us-ascii?q?mWwrOBd5Tqe2oD0yXHEkQEkB4c/WyANQcgAietuWXeDCZhFVj3eUPj7fF+qG+n?=
 =?us-ascii?q?Tk8z1wyKdFdu1761+x8Uhf2cTege0agCuCg8sTV0G1e90M/MB9WcoAphefYUXd?=
 =?us-ascii?q?RoxV5d1irivghsLI2mZ/R5j1oPYRVxl0ro2w9wC4kGms8v+jdiyAt0NLLd015b?=
 =?us-ascii?q?cT6c9Y7/N6eRKWTo+h2rLanM1QLwytGTr5wT5ew4plOrhwSgEk4v4j0zyNVO+2?=
 =?us-ascii?q?eN7ZXNSgwJWNT+VVhhpEsynK3TfiRov9Cc7nZrK6Th92aYg98=3D?=
Message-ID: <1516382386.2560.11.camel@tycho.nsa.gov>
Subject: Re: [PATCH] general protection fault in sock_has_perm
From:   Stephen Smalley <sds@tycho.nsa.gov>
To:     Mark Salyzyn <salyzyn@android.com>, linux-kernel@vger.kernel.org
Cc:     Paul Moore <paul@paul-moore.com>,
        Eric Paris <eparis@parisplace.org>,
        James Morris <james.l.morris@oracle.com>,
        "Serge E. Hallyn" <serge@hallyn.com>, selinux@tycho.nsa.gov,
        linux-security-module@vger.kernel.org, stable@vger.kernel.org
Date:   Fri, 19 Jan 2018 12:19:46 -0500
In-Reply-To: <20180118215853.228182-1-salyzyn@android.com>
References: <20180118215853.228182-1-salyzyn@android.com>
Organization: National Security Agency
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;