diff mbox series

selinux: fix inode_doinit_with_dentry() LABEL_INVALID error handling

Message ID 160453882404.5064.13236738388118581389.stgit@sifl (mailing list archive)
State Accepted
Headers show
Series selinux: fix inode_doinit_with_dentry() LABEL_INVALID error handling | expand

Commit Message

Paul Moore Nov. 5, 2020, 1:13 a.m. UTC
A previous fix, commit 83370b31a915 ("selinux: fix error initialization
in inode_doinit_with_dentry()"), changed how failures were handled
before a SELinux policy was loaded.  Unfortunately that patch was
potentially problematic for two reasons: it set the isec->initialized
state without holding a lock, and it didn't set the inode's SELinux
label to the "default" for the particular filesystem.  The later can
be a problem if/when a later attempt to revalidate the inode fails
and SELinux reverts to the existing inode label.

This patch should restore the default inode labeling that existed
before the original fix, without affecting the LABEL_INVALID marking
such that revalidation will still be attempted in the future.

Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()")
Reported-by: Sven Schnelle <svens@linux.ibm.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/selinux/hooks.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Comments

Ondrej Mosnacek Nov. 5, 2020, 8:55 a.m. UTC | #1
On Thu, Nov 5, 2020 at 2:13 AM Paul Moore <paul@paul-moore.com> wrote:
> A previous fix, commit 83370b31a915 ("selinux: fix error initialization
> in inode_doinit_with_dentry()"), changed how failures were handled
> before a SELinux policy was loaded.  Unfortunately that patch was
> potentially problematic for two reasons: it set the isec->initialized
> state without holding a lock, and it didn't set the inode's SELinux
> label to the "default" for the particular filesystem.  The later can
> be a problem if/when a later attempt to revalidate the inode fails
> and SELinux reverts to the existing inode label.
>
> This patch should restore the default inode labeling that existed
> before the original fix, without affecting the LABEL_INVALID marking
> such that revalidation will still be attempted in the future.
>
> Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()")
> Reported-by: Sven Schnelle <svens@linux.ibm.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/selinux/hooks.c |   31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)

FWIW, the patch looks good to me.

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Sven Schnelle Nov. 5, 2020, 9:03 a.m. UTC | #2
Ondrej Mosnacek <omosnace@redhat.com> writes:

> On Thu, Nov 5, 2020 at 2:13 AM Paul Moore <paul@paul-moore.com> wrote:
>> A previous fix, commit 83370b31a915 ("selinux: fix error initialization
>> in inode_doinit_with_dentry()"), changed how failures were handled
>> before a SELinux policy was loaded.  Unfortunately that patch was
>> potentially problematic for two reasons: it set the isec->initialized
>> state without holding a lock, and it didn't set the inode's SELinux
>> label to the "default" for the particular filesystem.  The later can
>> be a problem if/when a later attempt to revalidate the inode fails
>> and SELinux reverts to the existing inode label.
>>
>> This patch should restore the default inode labeling that existed
>> before the original fix, without affecting the LABEL_INVALID marking
>> such that revalidation will still be attempted in the future.
>>
>> Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()")
>> Reported-by: Sven Schnelle <svens@linux.ibm.com>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>>  security/selinux/hooks.c |   31 +++++++++++++------------------
>>  1 file changed, 13 insertions(+), 18 deletions(-)
>
> FWIW, the patch looks good to me.
>
> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>

I just tested it on s390, works fine.

Tested-by: Sven Schnelle <svens@linux.ibm.com>

Thanks
Sven
Paul Moore Nov. 6, 2020, 4 a.m. UTC | #3
On Thu, Nov 5, 2020 at 4:03 AM Sven Schnelle <svens@linux.ibm.com> wrote:
> Ondrej Mosnacek <omosnace@redhat.com> writes:

...

> > FWIW, the patch looks good to me.
> >
> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> I just tested it on s390, works fine.
>
> Tested-by: Sven Schnelle <svens@linux.ibm.com>

Thanks Sven and Ondrej for the testing and review help; I just merged
the patch into selinux/next.
diff mbox series

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 158fc47d8620..c46312710e73 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1451,13 +1451,7 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 			 * inode_doinit with a dentry, before these inodes could
 			 * be used again by userspace.
 			 */
-			isec->initialized = LABEL_INVALID;
-			/*
-			 * There is nothing useful to jump to the "out"
-			 * label, except a needless spin lock/unlock
-			 * cycle.
-			 */
-			return 0;
+			goto out_invalid;
 		}
 
 		rc = inode_doinit_use_xattr(inode, dentry, sbsec->def_sid,
@@ -1513,15 +1507,8 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 			 * inode_doinit() with a dentry, before these inodes
 			 * could be used again by userspace.
 			 */
-			if (!dentry) {
-				isec->initialized = LABEL_INVALID;
-				/*
-				 * There is nothing useful to jump to the "out"
-				 * label, except a needless spin lock/unlock
-				 * cycle.
-				 */
-				return 0;
-			}
+			if (!dentry)
+				goto out_invalid;
 			rc = selinux_genfs_get_sid(dentry, sclass,
 						   sbsec->flags, &sid);
 			if (rc) {
@@ -1546,11 +1533,10 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 out:
 	spin_lock(&isec->lock);
 	if (isec->initialized == LABEL_PENDING) {
-		if (!sid || rc) {
+		if (rc) {
 			isec->initialized = LABEL_INVALID;
 			goto out_unlock;
 		}
-
 		isec->initialized = LABEL_INITIALIZED;
 		isec->sid = sid;
 	}
@@ -1558,6 +1544,15 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 out_unlock:
 	spin_unlock(&isec->lock);
 	return rc;
+
+out_invalid:
+	spin_lock(&isec->lock);
+	if (isec->initialized == LABEL_PENDING) {
+		isec->initialized = LABEL_INVALID;
+		isec->sid = sid;
+	}
+	spin_unlock(&isec->lock);
+	return 0;
 }
 
 /* Convert a Linux signal to an access vector. */