| Message ID | 20150618133310.12722.16624.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/18/2015 09:33 AM, David Howells wrote: > Create a common helper function to determine the label for a new inode. This > is then used by: > > - may_create() > - selinux_dentry_init_security() > - selinux_inode_init_security() > - selinux_file_open_union() > > This will change the behaviour of the first two functions slightly, bringing > them into line with the third. The fourth function is newly created in a > preceding patch and applied only in the case of a filesystem union or overlay. > > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov> > Signed-off-by: David Howells <dhowells@redhat.com> > --- > > security/selinux/hooks.c | 124 ++++++++++++++++++++++------------------------ > 1 file changed, 60 insertions(+), 64 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index c4495a797eb1..7eef1032c11a 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1684,6 +1684,42 @@ out: > return rc; > } > > +/* > + * Determine the label for an inode that might be unioned. > + */ > +static int selinux_determine_inode_label(const struct inode *dir, > + const struct qstr *name, > + const char *caller, > + u16 tclass, > + u32 *_new_isid) > +{ > + const struct superblock_security_struct *sbsec; > + const struct inode_security_struct *dsec; > + const struct task_security_struct *tsec = current_security(); > + int rc; > + > + sbsec = dir->i_sb->s_security; > + > + if ((sbsec->flags & SE_SBINITIALIZED) && > + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { > + *_new_isid = sbsec->mntpoint_sid; > + } else if (tsec->create_sid) { This doesn't quite match the logic in inode_init_security today, see its checking of SBLABEL_MNT. > + *_new_isid = tsec->create_sid; > + } else { > + dsec = dir->i_security; > + > + rc = security_transition_sid(tsec->sid, dsec->sid, tclass, > + name, _new_isid); > + if (rc) { > + pr_warn("%s: security_transition_sid failed, rc=%d (name=%*.*s)\n", > + caller, -rc, name->len, name->len, name->name); Let's drop this pr_warn call. > + return rc; > + } > + } > + > + return 0; > +} > + > /* Check whether a task can create a file. */ > static int may_create(struct inode *dir, > struct dentry *dentry, > @@ -1700,7 +1736,6 @@ static int may_create(struct inode *dir, > sbsec = dir->i_sb->s_security; > > sid = tsec->sid; > - newsid = tsec->create_sid; > > ad.type = LSM_AUDIT_DATA_DENTRY; > ad.u.dentry = dentry; > @@ -1711,12 +1746,10 @@ static int may_create(struct inode *dir, > if (rc) > return rc; > > - if (!newsid || !(sbsec->flags & SBLABEL_MNT)) { > - rc = security_transition_sid(sid, dsec->sid, tclass, > - &dentry->d_name, &newsid); > - if (rc) > - return rc; > - } > + rc = selinux_determine_inode_label(dir, &dentry->d_name, __func__, > + tclass, &newsid); > + if (rc) > + return rc; > > rc = avc_has_perm(sid, newsid, tclass, FILE__CREATE, &ad); > if (rc) > @@ -2723,32 +2756,14 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, > struct qstr *name, void **ctx, > u32 *ctxlen) > { > - const struct cred *cred = current_cred(); > - struct task_security_struct *tsec; > - struct inode_security_struct *dsec; > - struct superblock_security_struct *sbsec; > - struct inode *dir = d_backing_inode(dentry->d_parent); > u32 newsid; > int rc; > > - tsec = cred->security; > - dsec = dir->i_security; > - sbsec = dir->i_sb->s_security; > - > - if (tsec->create_sid && sbsec->behavior != SECURITY_FS_USE_MNTPOINT) { > - newsid = tsec->create_sid; > - } else { > - rc = security_transition_sid(tsec->sid, dsec->sid, > - inode_mode_to_security_class(mode), > - name, > - &newsid); > - if (rc) { > - printk(KERN_WARNING > - "%s: security_transition_sid failed, rc=%d\n", > - __func__, -rc); > - return rc; > - } > - } > + rc = selinux_determine_inode_label(d_inode(dentry), name, __func__, > + inode_mode_to_security_class(mode), > + &newsid); > + if (rc) > + return rc; > > return security_sid_to_context(newsid, (char **)ctx, ctxlen); > } > @@ -2771,22 +2786,12 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, > sid = tsec->sid; > newsid = tsec->create_sid; > > - if ((sbsec->flags & SE_SBINITIALIZED) && > - (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) > - newsid = sbsec->mntpoint_sid; > - else if (!newsid || !(sbsec->flags & SBLABEL_MNT)) { > - rc = security_transition_sid(sid, dsec->sid, > - inode_mode_to_security_class(inode->i_mode), > - qstr, &newsid); > - if (rc) { > - printk(KERN_WARNING "%s: " > - "security_transition_sid failed, rc=%d (dev=%s " > - "ino=%ld)\n", > - __func__, > - -rc, inode->i_sb->s_id, inode->i_ino); > - return rc; > - } > - } > + rc = selinux_determine_inode_label( > + dir, qstr, __func__, > + inode_mode_to_security_class(inode->i_mode), > + &newsid); > + if (rc) > + return rc; > > /* Possibly defer initialization to selinux_complete_init. */ > if (sbsec->flags & SE_SBINITIALIZED) { > @@ -3502,9 +3507,7 @@ static int selinux_file_open_union(struct file *file, > struct file_security_struct *fsec, > const struct cred *cred) > { > - const struct superblock_security_struct *sbsec; > - const struct inode_security_struct *isec, *dsec, *fisec; > - const struct task_security_struct *tsec = current_security(); > + const struct inode_security_struct *isec, *fisec; > struct common_audit_data ad; > struct dentry *union_dentry = file->f_path.dentry; > const struct inode *union_inode = d_inode(union_dentry); > @@ -3512,29 +3515,22 @@ static int selinux_file_open_union(struct file *file, > struct dentry *dir; > int rc; > > - sbsec = union_dentry->d_sb->s_security; > - > if (union_inode) { > + /* If we're opening an overlay inode, use the label from that > + * in preference to the label on a lower inode which we might > + * actually be opening. > + */ > isec = union_inode->i_security; > fsec->union_isid = isec->sid; > - } else if ((sbsec->flags & SE_SBINITIALIZED) && > - (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { > - fsec->union_isid = sbsec->mntpoint_sid; > } else { > dir = dget_parent(union_dentry); > - dsec = d_inode(dir)->i_security; > - > - rc = security_transition_sid( > - tsec->sid, dsec->sid, > - inode_mode_to_security_class(lower_inode->i_mode), > + rc = selinux_determine_inode_label( > + d_inode(dir), > &union_dentry->d_name, > + __func__, > + inode_mode_to_security_class(lower_inode->i_mode), > &fsec->union_isid); > dput(dir); > - if (rc) { > - pr_warn("%s: security_transition_sid failed, rc=%d (name=%pD)\n", > - __func__, -rc, file); > - return rc; > - } > } > > /* We need to check that the union file is allowed to be opened as well > > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Smalley <sds@tycho.nsa.gov> wrote: > > + if ((sbsec->flags & SE_SBINITIALIZED) && > > + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { > > + *_new_isid = sbsec->mntpoint_sid; > > + } else if (tsec->create_sid) { > > This doesn't quite match the logic in inode_init_security today, see its > checking of SBLABEL_MNT. Fair point. What does SBLABEL_MNT mean precisely? It seems to indicate one of an odd mix of behaviours. I presume it means that we *have* to calculate a label and can't get one from the underlying fs if it is not set. Also, in: sbsec->flags |= SE_SBINITIALIZED; if (selinux_is_sblabel_mnt(sb)) sbsec->flags |= SBLABEL_MNT; should SE_SBINITIALIZED be set after SBLABEL_MNT? And should there be a memory barrier in here somewhere before the setting of SE_SBINITIALIZED? David -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/18/2015 11:13 AM, David Howells wrote: > Stephen Smalley <sds@tycho.nsa.gov> wrote: > >>> + if ((sbsec->flags & SE_SBINITIALIZED) && >>> + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { >>> + *_new_isid = sbsec->mntpoint_sid; >>> + } else if (tsec->create_sid) { >> >> This doesn't quite match the logic in inode_init_security today, see its >> checking of SBLABEL_MNT. > > Fair point. What does SBLABEL_MNT mean precisely? It seems to indicate one > of an odd mix of behaviours. I presume it means that we *have* to calculate a > label and can't get one from the underlying fs if it is not set. It means the filesystem supports per-file labeling and you can use setxattr(..."security.selinux") and setfscreatecon() for files on it. You can see whether it is set on a filesystem by looking for the seclabel option in cat /proc/mounts. If it is not set, then we ignore tsec->create_sid. It is arguable as to whether it is correct to always call security_transition_sid() there either, but that's another topic. > > Also, in: > > sbsec->flags |= SE_SBINITIALIZED; > if (selinux_is_sblabel_mnt(sb)) > sbsec->flags |= SBLABEL_MNT; > > should SE_SBINITIALIZED be set after SBLABEL_MNT? And should there be a > memory barrier in here somewhere before the setting of SE_SBINITIALIZED? I believe that's all under sbsec->lock held by the caller, but that code has changed a lot and been refactored significantly by others. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Smalley <sds@tycho.nsa.gov> wrote: > > Fair point. What does SBLABEL_MNT mean precisely? It seems to indicate one > > of an odd mix of behaviours. I presume it means that we *have* to calculate a > > label and can't get one from the underlying fs if it is not set. > > It means the filesystem supports per-file labeling and you can use > setxattr(..."security.selinux") and setfscreatecon() for files on it. > You can see whether it is set on a filesystem by looking for the > seclabel option in cat /proc/mounts. If it is not set, then we ignore > tsec->create_sid. It is arguable as to whether it is correct to always > call security_transition_sid() there either, but that's another topic. Okay, so how about the attached? David --- static int selinux_determine_inode_label(const struct inode *dir, const struct qstr *name, const char *caller, u16 tclass, u32 *_new_isid) { const struct superblock_security_struct *sbsec = dir->i_sb->s_security; const struct inode_security_struct *dsec = dir->i_security; const struct task_security_struct *tsec = current_security(); if ((sbsec->flags & SE_SBINITIALIZED) && (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { *_new_isid = sbsec->mntpoint_sid; } else if ((sbsec->flags & SBLABEL_MNT) && tsec->create_sid) { *_new_isid = tsec->create_sid; } else { return security_transition_sid(tsec->sid, dsec->sid, tclass, name, _new_isid); } return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/18/2015 11:32 AM, David Howells wrote: > Stephen Smalley <sds@tycho.nsa.gov> wrote: > >>> Fair point. What does SBLABEL_MNT mean precisely? It seems to indicate one >>> of an odd mix of behaviours. I presume it means that we *have* to calculate a >>> label and can't get one from the underlying fs if it is not set. >> >> It means the filesystem supports per-file labeling and you can use >> setxattr(..."security.selinux") and setfscreatecon() for files on it. >> You can see whether it is set on a filesystem by looking for the >> seclabel option in cat /proc/mounts. If it is not set, then we ignore >> tsec->create_sid. It is arguable as to whether it is correct to always >> call security_transition_sid() there either, but that's another topic. > > Okay, so how about the attached? > > David > --- > static int selinux_determine_inode_label(const struct inode *dir, > const struct qstr *name, > const char *caller, > u16 tclass, > u32 *_new_isid) > { > const struct superblock_security_struct *sbsec = dir->i_sb->s_security; > const struct inode_security_struct *dsec = dir->i_security; > const struct task_security_struct *tsec = current_security(); > > if ((sbsec->flags & SE_SBINITIALIZED) && > (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { > *_new_isid = sbsec->mntpoint_sid; > } else if ((sbsec->flags & SBLABEL_MNT) && > tsec->create_sid) { > *_new_isid = tsec->create_sid; > } else { > return security_transition_sid(tsec->sid, dsec->sid, tclass, > name, _new_isid); > } > > return 0; > } That looks good to me. In fact, I'd take a patch that defines that function and rewrites may_create(), inode_init_security(), and dentry_init_security() to use it even before (or independent of) the union support patches. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/18/2015 11:32 AM, David Howells wrote: > Stephen Smalley <sds@tycho.nsa.gov> wrote: > >>> Fair point. What does SBLABEL_MNT mean precisely? It seems to indicate one >>> of an odd mix of behaviours. I presume it means that we *have* to calculate a >>> label and can't get one from the underlying fs if it is not set. >> >> It means the filesystem supports per-file labeling and you can use >> setxattr(..."security.selinux") and setfscreatecon() for files on it. >> You can see whether it is set on a filesystem by looking for the >> seclabel option in cat /proc/mounts. If it is not set, then we ignore >> tsec->create_sid. It is arguable as to whether it is correct to always >> call security_transition_sid() there either, but that's another topic. > > Okay, so how about the attached? > > David > --- > static int selinux_determine_inode_label(const struct inode *dir, > const struct qstr *name, > const char *caller, > u16 tclass, > u32 *_new_isid) > { > const struct superblock_security_struct *sbsec = dir->i_sb->s_security; > const struct inode_security_struct *dsec = dir->i_security; > const struct task_security_struct *tsec = current_security(); > > if ((sbsec->flags & SE_SBINITIALIZED) && > (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { > *_new_isid = sbsec->mntpoint_sid; > } else if ((sbsec->flags & SBLABEL_MNT) && > tsec->create_sid) { > *_new_isid = tsec->create_sid; > } else { > return security_transition_sid(tsec->sid, dsec->sid, tclass, > name, _new_isid); > } > > return 0; > } Except you can drop the caller argument now. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index c4495a797eb1..7eef1032c11a 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1684,6 +1684,42 @@ out: return rc; } +/* + * Determine the label for an inode that might be unioned. + */ +static int selinux_determine_inode_label(const struct inode *dir, + const struct qstr *name, + const char *caller, + u16 tclass, + u32 *_new_isid) +{ + const struct superblock_security_struct *sbsec; + const struct inode_security_struct *dsec; + const struct task_security_struct *tsec = current_security(); + int rc; + + sbsec = dir->i_sb->s_security; + + if ((sbsec->flags & SE_SBINITIALIZED) && + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { + *_new_isid = sbsec->mntpoint_sid; + } else if (tsec->create_sid) { + *_new_isid = tsec->create_sid; + } else { + dsec = dir->i_security; + + rc = security_transition_sid(tsec->sid, dsec->sid, tclass, + name, _new_isid); + if (rc) { + pr_warn("%s: security_transition_sid failed, rc=%d (name=%*.*s)\n", + caller, -rc, name->len, name->len, name->name); + return rc; + } + } + + return 0; +} + /* Check whether a task can create a file. */ static int may_create(struct inode *dir, struct dentry *dentry, @@ -1700,7 +1736,6 @@ static int may_create(struct inode *dir, sbsec = dir->i_sb->s_security; sid = tsec->sid; - newsid = tsec->create_sid; ad.type = LSM_AUDIT_DATA_DENTRY; ad.u.dentry = dentry; @@ -1711,12 +1746,10 @@ static int may_create(struct inode *dir, if (rc) return rc; - if (!newsid || !(sbsec->flags & SBLABEL_MNT)) { - rc = security_transition_sid(sid, dsec->sid, tclass, - &dentry->d_name, &newsid); - if (rc) - return rc; - } + rc = selinux_determine_inode_label(dir, &dentry->d_name, __func__, + tclass, &newsid); + if (rc) + return rc; rc = avc_has_perm(sid, newsid, tclass, FILE__CREATE, &ad); if (rc) @@ -2723,32 +2756,14 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, struct qstr *name, void **ctx, u32 *ctxlen) { - const struct cred *cred = current_cred(); - struct task_security_struct *tsec; - struct inode_security_struct *dsec; - struct superblock_security_struct *sbsec; - struct inode *dir = d_backing_inode(dentry->d_parent); u32 newsid; int rc; - tsec = cred->security; - dsec = dir->i_security; - sbsec = dir->i_sb->s_security; - - if (tsec->create_sid && sbsec->behavior != SECURITY_FS_USE_MNTPOINT) { - newsid = tsec->create_sid; - } else { - rc = security_transition_sid(tsec->sid, dsec->sid, - inode_mode_to_security_class(mode), - name, - &newsid); - if (rc) { - printk(KERN_WARNING - "%s: security_transition_sid failed, rc=%d\n", - __func__, -rc); - return rc; - } - } + rc = selinux_determine_inode_label(d_inode(dentry), name, __func__, + inode_mode_to_security_class(mode), + &newsid); + if (rc) + return rc; return security_sid_to_context(newsid, (char **)ctx, ctxlen); } @@ -2771,22 +2786,12 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, sid = tsec->sid; newsid = tsec->create_sid; - if ((sbsec->flags & SE_SBINITIALIZED) && - (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) - newsid = sbsec->mntpoint_sid; - else if (!newsid || !(sbsec->flags & SBLABEL_MNT)) { - rc = security_transition_sid(sid, dsec->sid, - inode_mode_to_security_class(inode->i_mode), - qstr, &newsid); - if (rc) { - printk(KERN_WARNING "%s: " - "security_transition_sid failed, rc=%d (dev=%s " - "ino=%ld)\n", - __func__, - -rc, inode->i_sb->s_id, inode->i_ino); - return rc; - } - } + rc = selinux_determine_inode_label( + dir, qstr, __func__, + inode_mode_to_security_class(inode->i_mode), + &newsid); + if (rc) + return rc; /* Possibly defer initialization to selinux_complete_init. */ if (sbsec->flags & SE_SBINITIALIZED) { @@ -3502,9 +3507,7 @@ static int selinux_file_open_union(struct file *file, struct file_security_struct *fsec, const struct cred *cred) { - const struct superblock_security_struct *sbsec; - const struct inode_security_struct *isec, *dsec, *fisec; - const struct task_security_struct *tsec = current_security(); + const struct inode_security_struct *isec, *fisec; struct common_audit_data ad; struct dentry *union_dentry = file->f_path.dentry; const struct inode *union_inode = d_inode(union_dentry); @@ -3512,29 +3515,22 @@ static int selinux_file_open_union(struct file *file, struct dentry *dir; int rc; - sbsec = union_dentry->d_sb->s_security; - if (union_inode) { + /* If we're opening an overlay inode, use the label from that + * in preference to the label on a lower inode which we might + * actually be opening. + */ isec = union_inode->i_security; fsec->union_isid = isec->sid; - } else if ((sbsec->flags & SE_SBINITIALIZED) && - (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) { - fsec->union_isid = sbsec->mntpoint_sid; } else { dir = dget_parent(union_dentry); - dsec = d_inode(dir)->i_security; - - rc = security_transition_sid( - tsec->sid, dsec->sid, - inode_mode_to_security_class(lower_inode->i_mode), + rc = selinux_determine_inode_label( + d_inode(dir), &union_dentry->d_name, + __func__, + inode_mode_to_security_class(lower_inode->i_mode), &fsec->union_isid); dput(dir); - if (rc) { - pr_warn("%s: security_transition_sid failed, rc=%d (name=%pD)\n", - __func__, -rc, file); - return rc; - } } /* We need to check that the union file is allowed to be opened as well
Create a common helper function to determine the label for a new inode. This is then used by: - may_create() - selinux_dentry_init_security() - selinux_inode_init_security() - selinux_file_open_union() This will change the behaviour of the first two functions slightly, bringing them into line with the third. The fourth function is newly created in a preceding patch and applied only in the case of a filesystem union or overlay. Suggested-by: Stephen Smalley <sds@tycho.nsa.gov> Signed-off-by: David Howells <dhowells@redhat.com> --- security/selinux/hooks.c | 124 ++++++++++++++++++++++------------------------ 1 file changed, 60 insertions(+), 64 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html