Re: [PATCH 5/7] SELinux: Handle opening of a unioned file

[David Howells]

Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:

> >> Until a process writes to the file, we just want to use the lower inode
> >> label, right?
> > 
> > No.
> > 
> > There are two issues:
> > 
> >  (1) Non-fd accesses to an overlayfs file use the security label on the
> >      overlay inode, not the lower inode, even before copy up because they
> >      go through the inode ops of the overlayfs file first.
> > 
> >  (2) I'm told that we want the ability to have a different label on the
> >      upper file to that on the lower file.  This is trivial in overlayfs
> >      since you always have an overlay inode off which to hang the security
> >      label, but tricky with unionmount since you may only have a dentry.
> 
> I recall that being controversial.  I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently.  But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?

Actually, for overlayfs, I've made it such that the label off of the overlay
inode is used for the open file.  This label gets determined when the overlay
inode is set up and so is cached for the lifetime of the inode struct.  Like
this in the patch I posted:

+	if (inode) {
+		isec = inode->i_security;
+		fsec->union_isid = isec->sid;
+	} ...

where 'inode' is the overlay inode.  The subsequent bits:

+	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
+		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+		fsec->union_isid = sbsec->mntpoint_sid;
+	} else {
+		dir = dget_parent(union_path->dentry);
+		dsec = dir->d_inode->i_security;
+
+		rc = security_transition_sid(
+			tsec->sid, dsec->sid,
+			inode_mode_to_security_class(file_inode(file)->i_mode),
+			&union_path->dentry->d_name,
+			&fsec->union_isid);
+		dput(dir);
+		if (rc) {
+			printk(KERN_WARNING "%s:  "
+			       "security_transition_sid failed, rc=%d (name=%pD)\n",
+			       __func__, -rc, file);
+			return rc;
+		}
+	}

are for future unionmount support where there isn't an inode in the union
layer but only an inode in a lower layer.

In patch 7/7, if fsec->union_isid is non-zero in file_has_perm() then that is
used directly with avc_has_perm() in preference to calling inode_has_perm() on
the overlay or lower inodes.

Btw, is it correct that file operations must follow the label currently on the
inode rather than caching it in file_security_struct?

> >  (2) I'm told that we want the ability to have a different label on the
> >      upper file to that on the lower file.  This is trivial in overlayfs
> >      since you always have an overlay inode off which to hang the security
> >      label, but tricky with unionmount since you may only have a dentry.
> 
> I recall that being controversial.  I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently.  But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?

I'm hoping Eric Paris might address this.  As I understand it, we don't want
to give the container access to the lower layer labels, but would rather
consistently use the overlay labels.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/