Re: [PATCH-selinuxns] selinux: Annotate lockdep for services locks

[Paul Moore]

On Fri, Feb 2, 2018 at 9:40 AM, peter enderborg
 wrote:
> I will send the patches to pauls next tree. Im abit confused on when that is
> appropriate. Obviously there will be collisions with the namespace, but
> the patches also solves few of my prerequisite topics.

In an effort to clear up any confusion, here is the tentative plan ...
While I generally try to avoid rebasing the selinux/next branch, there
are a few things that we probably want to pull in from Linus' tree, so
after the merge window closes (I expect that to happen next weekend)
I'll rebase selinux/next to v4.16-rc1.  I always send a note to the
list when I rebase the selinux/next branch.

>From there, it's a matter of sorting out the merging logistics; I
suspect Stephen will probably just rebase his own selinuxns patches
(and presumably incorporate any feedback), but I tend to be a bit more
forgiving about manual fixups when merging patches after a rebase.
After Stephen's patches are merged we can merge the other bits that
rely on them.

Does that make sense?

-- 
paul moore
www.paul-moore.com

[PATCH 4.4 65/67] selinux: general protection fault in sock_has_perm

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Mark Salyzyn 

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:  [#1] PREEMPT SMP KASAN
CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
task: 8801d1095f00 task.stack: 8800b595
RIP: 0010:[]  [] sock_has_perm+0x1fe/0x3e0 
security/selinux/hooks.c:4069
RSP: 0018:8800b5957ce0  EFLAGS: 00010202
RAX: dc00 RBX: 110016b2af9f RCX: 81b69b51
RDX: 0002 RSI:  RDI: 0010
RBP: 8800b5957de0 R08: 0001 R09: 0001
R10:  R11: 110016b2af68 R12: 8800b5957db8
R13:  R14: 8800b7259f40 R15: 00d7
FS:  7f72f5ae2700() GS:8801db30() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 00a2fa38 CR3: 0001d798 CR4: 00160670
DR0:  DR1:  DR2: 
DR3:  DR6: fffe0ff0 DR7: 0400
Stack:
 81b69a1f 8800b5957d58 8000b5957d30 41b58ab3
 83fc82f2 81b69980 0246 8801d1096770
 8801d3165668 8157844b 8801d1095f00
 8801
Call Trace:
[] selinux_socket_setsockopt+0x4d/0x80 
security/selinux/hooks.c:4338
[] security_socket_setsockopt+0x7d/0xb0 
security/security.c:1257
[] SYSC_setsockopt net/socket.c:1757 [inline]
[] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
[] 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  [] sock_has_perm+0x1fe/0x3e0 
security/selinux/hooks.c:4069
RSP 
---[ end trace 7b5aaf788fef6174 ]---

Signed-off-by: Mark Salyzyn 
Acked-by: Paul Moore 
Cc: Eric Dumazet 
Cc: Stephen Smalley 
Cc: selinux@tycho.nsa.gov
Cc: linux-security-mod...@vger.kernel.org
Cc: Eric Paris 
Cc: Serge E. Hallyn 
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman 

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

Re: [PATCH-selinuxns] selinux: Annotate lockdep for services locks

[Stephen Smalley]

On Fri, 2018-02-02 at 15:40 +0100, peter enderborg wrote:
> Resent with sign-off.
> If you pick the patches that use the dynamic allocation of locks you
> can pick it.
> The patches for the annotation does not apply for the selinux-next at
> the moment
> so have to be for selinuxns.
> I will send the patches to pauls next tree. Im abit confused on when
> that is
> appropriate. Obviously there will be collisions with the namespace,
> but
> the patches also solves few of my prerequisite topics.

FWIW, I created a selinuxns-base branch with just my first three
commits and your commit (immediately after my first one, since it
doesn't depend on the other ones), based on current selinux/next.  My
first three commits are the ones that are purely restructuring and do
not have any effect on SELinux behavior or APIs (userspace or LSM
hooks, obviously SELinux-internal interfaces do change).  My fourth
commit on the selinuxns branch ("netns,selinux: create the selinux
netlink socket per network namespace") could perhaps also be added but
that one might be more controversial since it does change existing
SELinux behavior (although arguably the current behavior is wrong) and
in any event I don't think it has any impact on your patches. You can
probably base your work on top of selinuxns-base until those are either
flushed to selinux/next or definitively rejected, although obviously
they may have to change in response to review comments.  That branch
will however get re-based when selinux/next is re-based (to something
4.15 based).

> 
> 
> On 02/02/2018 03:10 PM, Stephen Smalley wrote:
> > On Fri, 2018-02-02 at 09:05 +0100, Peter Enderborg wrote:
> > > The locks are moved to dynamic allocation, we need to
> > > help the lockdep system to classify the locks.
> > > This adds to lockdep annotation for the page mutex and
> > > for the ss lock.
> > 
> > Thanks, but missing a Signed-off-by: line.  Also, just to be clear,
> > you
> > shouldn't re-base your work on top of the entire selinuxns branch,
> > since I only expect the first few patches to be mergeable in the
> > near
> > term.
> > 
> > I also will need to re-base again when the selinux next branch
> > moves to
> > something 4.15-based, including fixing up the bpf hooks that were
> > introduced there.
> > 
> > > ---
> > >  security/selinux/ss/services.c | 5 +
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/security/selinux/ss/services.c
> > > b/security/selinux/ss/services.c
> > > index abc5383..ba463c0 100644
> > > --- a/security/selinux/ss/services.c
> > > +++ b/security/selinux/ss/services.c
> > > @@ -70,6 +70,9 @@
> > >  #include "ebitmap.h"
> > >  #include "audit.h"
> > >  
> > > +static struct lock_class_key selinux_ss_class_key;
> > > +static struct lock_class_key selinux_status_class_key;
> > > +
> > >  /* Policy capability names */
> > >  char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
> > >   "network_peer_controls",
> > > @@ -88,7 +91,9 @@ int selinux_ss_create(struct selinux_ss **ss)
> > >   if (!newss)
> > >   return -ENOMEM;
> > >   rwlock_init(>policy_rwlock);
> > > + lockdep_set_class(>policy_rwlock,
> > > _ss_class_key);
> > >   mutex_init(>status_lock);
> > > + lockdep_set_class(>status_lock,
> > > _status_class_key);
> > >   *ss = newss;
> > >   return 0;
> > >  }
> 
> 
> 

Re: [PATCH-selinuxns] selinux: Annotate lockdep for services locks

[peter enderborg]

Resent with sign-off.
If you pick the patches that use the dynamic allocation of locks you can pick 
it.
The patches for the annotation does not apply for the selinux-next at the moment
so have to be for selinuxns.
I will send the patches to pauls next tree. Im abit confused on when that is
appropriate. Obviously there will be collisions with the namespace, but
the patches also solves few of my prerequisite topics.


On 02/02/2018 03:10 PM, Stephen Smalley wrote:
> On Fri, 2018-02-02 at 09:05 +0100, Peter Enderborg wrote:
>> The locks are moved to dynamic allocation, we need to
>> help the lockdep system to classify the locks.
>> This adds to lockdep annotation for the page mutex and
>> for the ss lock.
> Thanks, but missing a Signed-off-by: line.  Also, just to be clear, you
> shouldn't re-base your work on top of the entire selinuxns branch,
> since I only expect the first few patches to be mergeable in the near
> term.
>
> I also will need to re-base again when the selinux next branch moves to
> something 4.15-based, including fixing up the bpf hooks that were
> introduced there.
>
>> ---
>>  security/selinux/ss/services.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/security/selinux/ss/services.c
>> b/security/selinux/ss/services.c
>> index abc5383..ba463c0 100644
>> --- a/security/selinux/ss/services.c
>> +++ b/security/selinux/ss/services.c
>> @@ -70,6 +70,9 @@
>>  #include "ebitmap.h"
>>  #include "audit.h"
>>  
>> +static struct lock_class_key selinux_ss_class_key;
>> +static struct lock_class_key selinux_status_class_key;
>> +
>>  /* Policy capability names */
>>  char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
>>  "network_peer_controls",
>> @@ -88,7 +91,9 @@ int selinux_ss_create(struct selinux_ss **ss)
>>  if (!newss)
>>  return -ENOMEM;
>>  rwlock_init(>policy_rwlock);
>> +lockdep_set_class(>policy_rwlock,
>> _ss_class_key);
>>  mutex_init(>status_lock);
>> +lockdep_set_class(>status_lock,
>> _status_class_key);
>>  *ss = newss;
>>  return 0;
>>  }

[PATCH-selinuxns] selinux: Annotate lockdep for services locks

[Peter Enderborg]

The locks are moved to dynamic allocation, we need to
help the lockdep system to classify the locks.
This adds to lockdep annotation for the page mutex and
for the ss lock.

Signed-off-by: Peter Enderborg 
---
 security/selinux/ss/services.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index abc5383..ba463c0 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -70,6 +70,9 @@
 #include "ebitmap.h"
 #include "audit.h"
 
+static struct lock_class_key selinux_ss_class_key;
+static struct lock_class_key selinux_status_class_key;
+
 /* Policy capability names */
 char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
"network_peer_controls",
@@ -88,7 +91,9 @@ int selinux_ss_create(struct selinux_ss **ss)
if (!newss)
return -ENOMEM;
rwlock_init(>policy_rwlock);
+   lockdep_set_class(>policy_rwlock, _ss_class_key);
mutex_init(>status_lock);
+   lockdep_set_class(>status_lock, _status_class_key);
*ss = newss;
return 0;
 }
-- 
2.7.4

Re: [PATCH-selinuxns] selinux: Annotate lockdep for services locks

[Stephen Smalley]

On Fri, 2018-02-02 at 09:05 +0100, Peter Enderborg wrote:
> The locks are moved to dynamic allocation, we need to
> help the lockdep system to classify the locks.
> This adds to lockdep annotation for the page mutex and
> for the ss lock.

Thanks, but missing a Signed-off-by: line.  Also, just to be clear, you
shouldn't re-base your work on top of the entire selinuxns branch,
since I only expect the first few patches to be mergeable in the near
term.

I also will need to re-base again when the selinux next branch moves to
something 4.15-based, including fixing up the bpf hooks that were
introduced there.

> ---
>  security/selinux/ss/services.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/security/selinux/ss/services.c
> b/security/selinux/ss/services.c
> index abc5383..ba463c0 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -70,6 +70,9 @@
>  #include "ebitmap.h"
>  #include "audit.h"
>  
> +static struct lock_class_key selinux_ss_class_key;
> +static struct lock_class_key selinux_status_class_key;
> +
>  /* Policy capability names */
>  char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
>   "network_peer_controls",
> @@ -88,7 +91,9 @@ int selinux_ss_create(struct selinux_ss **ss)
>   if (!newss)
>   return -ENOMEM;
>   rwlock_init(>policy_rwlock);
> + lockdep_set_class(>policy_rwlock,
> _ss_class_key);
>   mutex_init(>status_lock);
> + lockdep_set_class(>status_lock,
> _status_class_key);
>   *ss = newss;
>   return 0;
>  }

Re: [PATCH v2] general protection fault in sock_has_perm

[Greg KH]

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:  [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
> task: 8801d1095f00 task.stack: 8800b595
> RIP: 0010:[]  [] 
> sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
> RSP: 0018:8800b5957ce0  EFLAGS: 00010202
> RAX: dc00 RBX: 110016b2af9f RCX: 81b69b51
> RDX: 0002 RSI:  RDI: 0010
> RBP: 8800b5957de0 R08: 0001 R09: 0001
> R10:  R11: 110016b2af68 R12: 8800b5957db8
> R13:  R14: 8800b7259f40 R15: 00d7
> FS:  7f72f5ae2700() GS:8801db30() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 00a2fa38 CR3: 0001d798 CR4: 00160670
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> Stack:
>  81b69a1f 8800b5957d58 8000b5957d30 41b58ab3
>  83fc82f2 81b69980 0246 8801d1096770
>  8801d3165668 8157844b 8801d1095f00
>  8801
> Call Trace:
> [] selinux_socket_setsockopt+0x4d/0x80 
> security/selinux/hooks.c:4338
> [] security_socket_setsockopt+0x7d/0xb0 
> security/security.c:1257
> [] SYSC_setsockopt net/socket.c:1757 [inline]
> [] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
> [] 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  [] sock_has_perm+0x1fe/0x3e0 
> security/selinux/hooks.c:4069
> RSP 
> ---[ end trace 7b5aaf788fef6174 ]---
> 
> Signed-off-by: Mark Salyzyn 
> Signed-off-by: Paul Moore 
> Signed-off-by: Greg KH 
> Cc: Eric Dumazet 
> Cc: Stephen Smalley 
> Cc: selinux@tycho.nsa.gov
> Cc: linux-security-mod...@vger.kernel.org
> Cc: Eric Paris 
> Cc: Serge E. Hallyn 
> Cc: stable  # 3.18, 4.4
> Cc: linux-ker...@vger.kernel.org
> ---
> v2: return -EFAULT for null sk_security instead of 0

Now queued up, thanks.

greg k-h

Re: [SELinuxProject/selinux] gui: remove selinux-sepolgengui (#77)

[Petr Lautrbach]

On Thu, Jan 25, 2018 at 01:58:46PM -0800, Nicolas Iooss wrote:
> Hi,
> I sent a few hours ago these two patches on the mailing list, by as the first 
> one seems to be blocked somewhere (I have only received back the second one), 
> I am publishing them on Github too, as a Pull Request.
> 
> These patches removes selinux-sepolgengui because this application is not 
> compatible with Gtk3, Python 3 (it requires PyGTK), etc. and would otherwise 
> require some effort to update it.
> More precisely, even though ``pygi-convert.sh`` updated the code in order to 
> try making the application compatible with Python 3, PyGI... (in commit 
> 0f3beeb00e7a42cc2f44ef0392b8a3a7566a17d7), ``polgen.glade`` is still 
> incompatible with Gtk-Builder (it would need to be converted). I do not want 
> to spend time converting this file when I see that a bug preventing this 
> application to launch has been present for more than one year.


> polgengui.py is not compatible with Gtk3, Python 3, etc. Moreover it
> fails to load at least since the release 2.6-rc1:
> 
> $ python2 /usr/share/system-config-selinux/polgengui.py
> Traceback (most recent call last):
>   File "/usr/share/system-config-selinux/polgengui.py", line 778, in 
> app = childWindow()
>   File "/usr/share/system-config-selinux/polgengui.py", line 205, in __init__
> self.all_types = sepolicy.generate.get_all_types()
> AttributeError: 'module' object has no attribute 'generate'

This particular problem was fixed in Fedora by 
https://github.com/fedora-selinux/selinux/commit/ecd050d12cde54f3d7a31029be8582223d3d6eba

I must have forgotten to re-send it. Sorry.

> In fact, this bug has been introduced more than a year ago by commit
> b43991f ("policycoreutils: import sepolicy directly"), which
> replaced "from sepolicy import generate" with "import sepolicy" instead
> of "import sepolicy.generate".
> 
> As no one seems to have noticed, this application seems to be no longer
> used. Remove it from gui/.


I'd like to ask to postpone accepting this patch for another 2
weeks. I'll try to port/convert polgengui.py to be compatible with
Gtk-Builder. I'll come back either with patch or with Ack to drop it.

Thanks,

Petr


> Cheers
> You can view, comment on, or merge this pull request online at:
> 
>   https://github.com/SELinuxProject/selinux/pull/77
> 
> -- Commit Summary --
> 
>   * gui: remove selinux-polgengui application
>   * gui: remove "new" button in Modules page
> 
> -- File Changes --
> 
> M gui/Makefile (5)
> M gui/modulesPage.py (8)
> D gui/polgen.glade (2468)
> D gui/polgengui.py (779)
> D gui/selinux-polgengui.8 (35)
> D gui/selinux-polgengui.desktop (66)
> M gui/system-config-selinux.ui (12)
> 
> -- Patch Links --
> 
> https://github.com/SELinuxProject/selinux/pull/77.patch
> https://github.com/SELinuxProject/selinux/pull/77.diff
> 
> -- 
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly or view it on GitHub:
> https://github.com/SELinuxProject/selinux/pull/77


signature.asc
Description: PGP signature

[PATCH-selinuxns] selinux: Annotate lockdep for services locks

[Peter Enderborg]

The locks are moved to dynamic allocation, we need to
help the lockdep system to classify the locks.
This adds to lockdep annotation for the page mutex and
for the ss lock.
---
 security/selinux/ss/services.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index abc5383..ba463c0 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -70,6 +70,9 @@
 #include "ebitmap.h"
 #include "audit.h"
 
+static struct lock_class_key selinux_ss_class_key;
+static struct lock_class_key selinux_status_class_key;
+
 /* Policy capability names */
 char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
"network_peer_controls",
@@ -88,7 +91,9 @@ int selinux_ss_create(struct selinux_ss **ss)
if (!newss)
return -ENOMEM;
rwlock_init(>policy_rwlock);
+   lockdep_set_class(>policy_rwlock, _ss_class_key);
mutex_init(>status_lock);
+   lockdep_set_class(>status_lock, _status_class_key);
*ss = newss;
return 0;
 }
-- 
2.7.4