[Ocfs2-devel] [PATCH v3 0/7] Inode security label invalidation

[Ocfs2-devel] [PATCH v3 0/7] Inode security label invalidation

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Here is another version of the patch queue to make gfs2 and similar file
> systems work with SELinux.  As suggested by Stephen Smalley [*], the relevant
> uses of inode->security are wrapped in function calls that try to revalidate
> invalid labels.
>
>    [*] http://marc.info/?l=linux-kernel&m=144416710207686&w=2
>
> The patches are looking good from my point of view; is there anything else that
> needs addressing?
>
> Does SELinux have test suites that these patches could be tested agains?

git clone https://github.com/SELinuxProject/selinux-testsuite
sudo yum install perl-Test perl-Test-Harness selinux-policy-devel gcc 
libselinux-devel net-tools netlabel_tools iptables
cd selinux-testsuite
sudo make test

>
> Thanks,
> Andreas
>
> Andreas Gruenbacher (7):
>    selinux: Remove unused variable in selinux_inode_init_security
>    selinux: Add accessor functions for inode->i_security
>    selinux: Get rid of file_path_has_perm
>    selinux: Push dentry down from {dentry,path,file}_has_perm
>    security: Add hook to invalidate inode security labels
>    selinux: Revalidate invalid inode security labels
>    gfs2: Invalide security labels of inodes when they go invalid
>
>   fs/gfs2/glops.c                   |   2 +
>   include/linux/lsm_hooks.h         |   6 ++
>   include/linux/security.h          |   5 +
>   security/security.c               |   8 ++
>   security/selinux/hooks.c          | 213 ++++++++++++++++++++++----------------
>   security/selinux/include/objsec.h |   6 ++
>   6 files changed, 152 insertions(+), 88 deletions(-)
>

[Ocfs2-devel] [PATCH v3 1/7] selinux: Remove unused variable in selinux_inode_init_security

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/hooks.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e4369d8..fc8f626 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2756,13 +2756,11 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>   				       void **value, size_t *len)
>   {
>   	const struct task_security_struct *tsec = current_security();
> -	struct inode_security_struct *dsec;
>   	struct superblock_security_struct *sbsec;
>   	u32 sid, newsid, clen;
>   	int rc;
>   	char *context;
>
> -	dsec = dir->i_security;
>   	sbsec = dir->i_sb->s_security;
>
>   	sid = tsec->sid;
>

[Ocfs2-devel] [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235
Author: David Howells <dhowells@redhat.com>
Date:   Thu Jun 13 23:37:55 2013 +0100

     SELinux: Institute file_path_has_perm()

     Create a file_path_has_perm() function that is like path_has_perm() but
     instead takes a file struct that is the source of both the path and the
     inode (rather than getting the inode from the dentry in the path). 
  This
     is then used where appropriate.

     This will be useful for situations like unionmount where it will be
     possible to have an apparently-negative dentry (eg. a fallthrough) 
that is
     open with the file struct pointing to an inode on the lower fs.

     Signed-off-by: David Howells <dhowells@redhat.com>
     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

which I think David was intending to use as part of his 
SELinux/overlayfs support.

path_has_perm() uses d_backing_inode(path->dentry), while 
file_path_has_perm() uses file_inode(file).

>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   security/selinux/hooks.c | 18 +++---------------
>   1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 65e8689..d6b4dc9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1673,18 +1673,6 @@ static inline int path_has_perm(const struct cred *cred,
>   	return inode_has_perm(cred, inode, av, &ad);
>   }
>
> -/* Same as path_has_perm, but uses the inode from the file struct. */
> -static inline int file_path_has_perm(const struct cred *cred,
> -				     struct file *file,
> -				     u32 av)
> -{
> -	struct common_audit_data ad;
> -
> -	ad.type = LSM_AUDIT_DATA_PATH;
> -	ad.u.path = file->f_path;
> -	return inode_has_perm(cred, file_inode(file), av, &ad);
> -}
> -
>   /* Check whether a task can use an open file descriptor to
>      access an inode in a given way.  Check access to the
>      descriptor itself, and then use dentry_has_perm to
> @@ -2371,14 +2359,14 @@ static inline void flush_unauthorized_files(const struct cred *cred,
>   			struct tty_file_private *file_priv;
>
>   			/* Revalidate access to controlling tty.
> -			   Use file_path_has_perm on the tty path directly
> +			   Use path_has_perm on the tty path directly
>   			   rather than using file_has_perm, as this particular
>   			   open file may belong to another process and we are
>   			   only interested in the inode-based check here. */
>   			file_priv = list_first_entry(&tty->tty_files,
>   						struct tty_file_private, list);
>   			file = file_priv->file;
> -			if (file_path_has_perm(cred, file, FILE__READ | FILE__WRITE))
> +			if (path_has_perm(cred, &file->f_path, FILE__READ | FILE__WRITE))
>   				drop_tty = 1;
>   		}
>   		spin_unlock(&tty_files_lock);
> @@ -3537,7 +3525,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>   	 * new inode label or new policy.
>   	 * This check is not redundant - do not remove.
>   	 */
> -	return file_path_has_perm(cred, file, open_file_to_av(file));
> +	return path_has_perm(cred, &file->f_path, open_file_to_av(file));
>   }
>
>   /* task security operations */
>

[Ocfs2-devel] [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/28/2015 07:48 AM, Andreas Gruenbacher wrote:
> On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>>
>>> Use path_has_perm directly instead.
>>
>>
>> This reverts:
>>
>> commit 13f8e9810bff12d01807b6f92329111f45218235
>> Author: David Howells <dhowells@redhat.com>
>> Date:   Thu Jun 13 23:37:55 2013 +0100
>>
>>     SELinux: Institute file_path_has_perm()
>>
>>     Create a file_path_has_perm() function that is like path_has_perm() but
>>     instead takes a file struct that is the source of both the path and the
>>     inode (rather than getting the inode from the dentry in the path).  This
>>     is then used where appropriate.
>>
>>     This will be useful for situations like unionmount where it will be
>>     possible to have an apparently-negative dentry (eg. a fallthrough) that
>> is
>>     open with the file struct pointing to an inode on the lower fs.
>>
>>     Signed-off-by: David Howells <dhowells@redhat.com>
>>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>
>> which I think David was intending to use as part of his SELinux/overlayfs
>> support.
> 
> Okay. As long as overlayfs support in SELinux is in half-finished
> state, let's leave this alone.

Also, the caller is holding a spinlock (tty_files_lock), so you can't call inode_doinit from
here.

Try stress testing your patch series by just always setting isec->initialized to LABEL_INVALID.
Previously the *has_perm functions could be called under essentially any condition, with the exception
of when in a RCU walk and needing to audit the dname (but they did not previously block/sleep).

[Ocfs2-devel] [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/28/2015 01:31 PM, Stephen Smalley wrote:
> On 10/28/2015 07:48 AM, Andreas Gruenbacher wrote:
>> On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>>>
>>>> Use path_has_perm directly instead.
>>>
>>>
>>> This reverts:
>>>
>>> commit 13f8e9810bff12d01807b6f92329111f45218235
>>> Author: David Howells <dhowells@redhat.com>
>>> Date:   Thu Jun 13 23:37:55 2013 +0100
>>>
>>>      SELinux: Institute file_path_has_perm()
>>>
>>>      Create a file_path_has_perm() function that is like path_has_perm() but
>>>      instead takes a file struct that is the source of both the path and the
>>>      inode (rather than getting the inode from the dentry in the path).  This
>>>      is then used where appropriate.
>>>
>>>      This will be useful for situations like unionmount where it will be
>>>      possible to have an apparently-negative dentry (eg. a fallthrough) that
>>> is
>>>      open with the file struct pointing to an inode on the lower fs.
>>>
>>>      Signed-off-by: David Howells <dhowells@redhat.com>
>>>      Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>>
>>> which I think David was intending to use as part of his SELinux/overlayfs
>>> support.
>>
>> Okay. As long as overlayfs support in SELinux is in half-finished
>> state, let's leave this alone.
>
> Also, the caller is holding a spinlock (tty_files_lock), so you can't call inode_doinit from
> here.
>
> Try stress testing your patch series by just always setting isec->initialized to LABEL_INVALID.
> Previously the *has_perm functions could be called under essentially any condition, with the exception
> of when in a RCU walk and needing to audit the dname (but they did not previously block/sleep).

file_has_perm() also gets called from match_file() callback to 
iterate_fd(), which holds files->file_lock.

[Ocfs2-devel] [PATCH v3 2/7] selinux: Add accessor functions for inode->i_security

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Add functions dentry_security and inode_security for accessing
> inode->i_security.  These functions initially don't do much, but they
> will later be used to revalidate the security labels when necessary.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   security/selinux/hooks.c | 101 ++++++++++++++++++++++++++---------------------
>   1 file changed, 57 insertions(+), 44 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fc8f626..65e8689 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -241,6 +241,24 @@ static int inode_alloc_security(struct inode *inode)
>   	return 0;
>   }
>
> +/*
> + * Get the security label of a dentry's inode.
> + */
> +static struct inode_security_struct *dentry_security(struct dentry *dentry)
> +{
> +	struct inode *inode = d_backing_inode(dentry);
> +
> +	return inode->i_security;
> +}
> +
> +/*
> + * Get the security label of an inode.
> + */
> +static struct inode_security_struct *inode_security(struct inode *inode)
> +{
> +	return inode->i_security;
> +}
> +
>   static void inode_free_rcu(struct rcu_head *head)
>   {
>   	struct inode_security_struct *isec;
<snip>
> @@ -2207,7 +2222,6 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>   	struct task_security_struct *new_tsec;
>   	struct inode_security_struct *isec;
>   	struct common_audit_data ad;
> -	struct inode *inode = file_inode(bprm->file);
>   	int rc;
>
>   	/* SELinux context only depends on initial program or script and not
> @@ -2217,7 +2231,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>
>   	old_tsec = current_security();
>   	new_tsec = bprm->cred->security;
> -	isec = inode->i_security;
> +	isec = dentry_security(bprm->file->f_path.dentry);

IIUC, this could change which inode label gets used when using overlayfs 
(the overlay inode or the underlying inode).  Not sure whether the 
current code is correct for overlayfs (overlayfs + SELinux support still 
in progress).

> @@ -3154,7 +3168,7 @@ out_nofree:
>   static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>   				     const void *value, size_t size, int flags)
>   {
> -	struct inode_security_struct *isec = inode->i_security;
> +	struct inode_security_struct *isec = inode_security(inode);

Was it intentional to not do this for selinux_inode_getsecurity() and 
selinux_inode_getsecid()?

> @@ -3241,8 +3254,8 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
>   {
>   	struct common_audit_data ad;
>   	struct file_security_struct *fsec = file->f_security;
> -	struct inode *inode = file_inode(file);
> -	struct inode_security_struct *isec = inode->i_security;
> +	struct dentry *dentry = file->f_path.dentry;
> +	struct inode_security_struct *isec = dentry_security(dentry);
>   	struct lsm_ioctlop_audit ioctl;
>   	u32 ssid = cred_sid(cred);
>   	int rc;
> @@ -3263,7 +3276,7 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
>   			goto out;
>   	}
>
> -	if (unlikely(IS_PRIVATE(inode)))
> +	if (unlikely(IS_PRIVATE(dentry->d_inode)))
>   		return 0;
>
>   	rc = avc_has_extended_perms(ssid, isec->sid, isec->sclass,
> @@ -3506,7 +3519,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>   	struct inode_security_struct *isec;
>
>   	fsec = file->f_security;
> -	isec = file_inode(file)->i_security;
> +	isec = dentry_security(file->f_path.dentry);

Similarly for these cases, switching from file_inode(file) to 
d_backing_inode(dentry) could affect overlayfs interaction IIUC.  cc'd 
David for clarification.

[Ocfs2-devel] [PATCH v3 5/7] security: Add hook to invalidate inode security labels

[James Morris]

On Mon, 26 Oct 2015, Andreas Gruenbacher wrote:

> Add a hook to invalidate an inode's security label when the cached
> information becomes invalid.
> 
> Implement the new hook in selinux: set a flag when a security label becomes
> invalid.  When hitting a security label which has been marked as invalid in
> inode_has_perm, try reloading the label.
> 
> If an inode does not have any dentries attached, we cannot reload its
> security label because we cannot use the getxattr inode operation.  In that
> case, continue using the old, invalid label until a dentry becomes
> available.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<jmorris@namei.org>

[Ocfs2-devel] [PATCH v3 5/7] security: Add hook to invalidate inode security labels

[James Morris]

On Mon, 26 Oct 2015, Andreas Gruenbacher wrote:

> Add a hook to invalidate an inode's security label when the cached
> information becomes invalid.
> 
> Implement the new hook in selinux: set a flag when a security label becomes
> invalid.  When hitting a security label which has been marked as invalid in
> inode_has_perm, try reloading the label.
> 
> If an inode does not have any dentries attached, we cannot reload its
> security label because we cannot use the getxattr inode operation.  In that
> case, continue using the old, invalid label until a dentry becomes
> available.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<jmorris@namei.org>