[PATCH] selinux: fix inode security list corruption

[PATCH] selinux: fix inode security list corruption

[Stephen Smalley]

sb_finish_set_opts() can race with inode_free_security()
when initializing inode security structures for inodes
created prior to initial policy load or by the filesystem
during ->mount().   This appears to have always been
a possible race, but commit 3dc91d4 ("SELinux:  Fix possible
NULL pointer dereference in selinux_inode_permission()")
made it more evident by immediately reusing the unioned
list/rcu element  of the inode security structure for call_rcu()
upon an inode_free_security().  But the underlying issue
was already present before that commit as a possible use-after-free
of isec.

Shivnandan Kumar reported the list corruption and proposed
a patch to split the list and rcu elements out of the union
as separate fields of the inode_security_struct so that setting
the rcu element would not affect the list element.  However,
this would merely hide the issue and not truly fix the code.

This patch instead moves up the deletion of the list entry
prior to dropping the sbsec->isec_lock initially.  Then,
if the inode is dropped subsequently, there will be no further
references to the isec.

Reported-by: Shivnandan Kumar <shivnandan.k@samsung.com>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 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 b0e9404..e03bad5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -481,6 +481,7 @@ next_inode:
 				list_entry(sbsec->isec_head.next,
 					   struct inode_security_struct, list);
 		struct inode *inode = isec->inode;
+		list_del_init(&isec->list);
 		spin_unlock(&sbsec->isec_lock);
 		inode = igrab(inode);
 		if (inode) {
@@ -489,7 +490,6 @@ next_inode:
 			iput(inode);
 		}
 		spin_lock(&sbsec->isec_lock);
-		list_del_init(&isec->list);
 		goto next_inode;
 	}
 	spin_unlock(&sbsec->isec_lock);
-- 
1.8.3.1

Re: [PATCH] selinux: fix inode security list corruption

[Paul Moore]

On Thursday, October 02, 2014 01:00:25 PM Stephen Smalley wrote:
> sb_finish_set_opts() can race with inode_free_security()
> when initializing inode security structures for inodes
> created prior to initial policy load or by the filesystem
> during ->mount().   This appears to have always been
> a possible race, but commit 3dc91d4 ("SELinux:  Fix possible
> NULL pointer dereference in selinux_inode_permission()")
> made it more evident by immediately reusing the unioned
> list/rcu element  of the inode security structure for call_rcu()
> upon an inode_free_security().  But the underlying issue
> was already present before that commit as a possible use-after-free
> of isec.
> 
> Shivnandan Kumar reported the list corruption and proposed
> a patch to split the list and rcu elements out of the union
> as separate fields of the inode_security_struct so that setting
> the rcu element would not affect the list element.  However,
> this would merely hide the issue and not truly fix the code.
> 
> This patch instead moves up the deletion of the list entry
> prior to dropping the sbsec->isec_lock initially.  Then,
> if the inode is dropped subsequently, there will be no further
> references to the isec.
> 
> Reported-by: Shivnandan Kumar <shivnandan.k@samsung.com>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>  security/selinux/hooks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks for taking the time to track this down and submit a patch.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b0e9404..e03bad5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -481,6 +481,7 @@ next_inode:
>  				list_entry(sbsec->isec_head.next,
>  					   struct inode_security_struct, list);
>  		struct inode *inode = isec->inode;
> +		list_del_init(&isec->list);
>  		spin_unlock(&sbsec->isec_lock);
>  		inode = igrab(inode);
>  		if (inode) {
> @@ -489,7 +490,6 @@ next_inode:
>  			iput(inode);
>  		}
>  		spin_lock(&sbsec->isec_lock);
> -		list_del_init(&isec->list);
>  		goto next_inode;
>  	}
>  	spin_unlock(&sbsec->isec_lock);

-- 
paul moore
www.paul-moore.com