On Tue, Nov 3, 2020 at 2:02 PM Sven Schnelle <svens@xxxxxxxxxxxxx> wrote:
> Thanks for the patch. Unfortunately it doesn't seem to change anything
> for me. I can take a look into this tomorrow, but i don't know much
> about the internals of selinux, so i'm not sure whether i'm of much help.
I'm sorry that patch didn't work out. I just spent some more time
looking at the code+patch and the only other thing that I can see is
that if we mark the isec invalid, we don't bother setting the
isec->sid value to whatever default we may have already found. In a
perfect world this shouldn't matter, but if for whatever reason the
kernel can't revalidate the inode's label when it tries later it will
fallback to that default isec->sid.
I'm sorry to ask this again, but would you be able to test the attached patch?
--
paul moore
www.paul-moore.com
selinux: fix inode_doinit_with_dentry() error case locking
From: Paul Moore <paul@xxxxxxxxxxxxxx>
XXX - testing only patch, work in progress
Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()")
Reported-by: Sven Schnelle <svens@xxxxxxxxxxxxx>
Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
---
security/selinux/hooks.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
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. */
