On 06/16/2015 12:49 PM, David Howells wrote: > Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > >> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed >> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT, >> and this change was never propagated to selinux_dentry_init_security(). >> However, that commit also did not update >> security/selinux/hooks.c:may_create()'s logic for computing the new file >> label when checking CREATE permission, and therefore introduced a >> potential inconsistency between the label used for the permission check >> and the label assigned to the inode. >> >> That's why I suggested that we need a common helper for all three to >> ensure consistency there. > > I think a common helper is harder than it seems. We need the parent dir in > one of the cases the helper has to consider, but finding it is done in three > different ways, depending on the caller: > > (1) dentry_init can just use ->d_parent as there's a lock held that prevents > it changing (I think). This could use (2) instead, however. > > (2) file_open has to use dget_parent(). > > (3) inode_init doesn't have any dentries, but rather has the object and > parent inodes. > > If we don't mind file_open() always calling dget_parent(), then the common > helper can take the dir inode. > > Also, thinking ahead to the possibility of bringing unionmount into the kernel > at some point: union non-dir dentries that are not yet copied up have no inode > attached, but rather fall through to the underlying lower inode in the VFS. > This, however, gives us nowhere to hang the inode label. How expensive is the > security_transition_sid() call? Why are you talking about file_open()? It is may_create() that has the duplicated logic, which has the parent dir passed to it by its callers (selinux_inode_create, selinux_inode_symlink, selinux_inode_mkdir, selinux_mknod). selinux_inode_init_security() also gets passed the parent dir directly. Only selinux_dentry_init_security() has to use d_parent, which as you say is safe, so it can just pass the resulting dir to the helper. Until a process writes to the file, we just want to use the lower inode label, right? At the point a process writes to the file and a copy-up is produced, we could perform a one-time computation, and then once the upper inode is created, it should get set accordingly. So I wouldn't think we would need to call security_transition_sid() frequently. If so, we might want to do what was previously done in the userspace AVC in libselinux, and start caching security_transition_sid() results in the AVC itself and add an avc_transition_sid() interface (in the userspace AVC, this is avc_compute_create()). -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
