From b754026bd98e644f9337224ffd4201e02dfe1c43 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Fri, 22 Feb 2019 15:57:14 +0100 Subject: selinux: try security xattr after genfs for kernfs filesystems Since kernfs supports the security xattr handlers, we can simply use these to determine the inode's context, dropping the need to update it from kernfs explicitly using a security_inode_notifysecctx() call. We achieve this by setting a new sbsec flag SE_SBGENFS_XATTR to all mounts that are known to use kernfs under the hood and then fetching the xattrs after determining the fallback genfs sid in inode_doinit_with_dentry() when this flag is set. This will allow implementing full security xattr support in kernfs and removing the ...notifysecctx() call in a subsequent patch. Signed-off-by: Ondrej Mosnacek Acked-by: Stephen Smalley Acked-by: Casey Schaufler [PM: more manual merge fixups] Signed-off-by: Paul Moore --- security/selinux/hooks.c | 157 +++++++++++++++++++++++++---------------------- 1 file changed, 84 insertions(+), 73 deletions(-) (limited to 'security/selinux/hooks.c') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 1d0b37af2444..085409b36794 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -751,11 +751,13 @@ static int selinux_set_mnt_opts(struct super_block *sb, if (!strcmp(sb->s_type->name, "debugfs") || !strcmp(sb->s_type->name, "tracefs") || - !strcmp(sb->s_type->name, "sysfs") || - !strcmp(sb->s_type->name, "pstore") || + !strcmp(sb->s_type->name, "pstore")) + sbsec->flags |= SE_SBGENFS; + + if (!strcmp(sb->s_type->name, "sysfs") || !strcmp(sb->s_type->name, "cgroup") || !strcmp(sb->s_type->name, "cgroup2")) - sbsec->flags |= SE_SBGENFS; + sbsec->flags |= SE_SBGENFS | SE_SBGENFS_XATTR; if (!sbsec->behavior) { /* @@ -1354,6 +1356,67 @@ static int selinux_genfs_get_sid(struct dentry *dentry, return rc; } +static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry, + u32 def_sid, u32 *sid) +{ +#define INITCONTEXTLEN 255 + char *context; + unsigned int len; + int rc; + + len = INITCONTEXTLEN; + context = kmalloc(len + 1, GFP_NOFS); + if (!context) + return -ENOMEM; + + context[len] = '\0'; + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len); + if (rc == -ERANGE) { + kfree(context); + + /* Need a larger buffer. Query for the right size. */ + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, NULL, 0); + if (rc < 0) + return rc; + + len = rc; + context = kmalloc(len + 1, GFP_NOFS); + if (!context) + return -ENOMEM; + + context[len] = '\0'; + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, + context, len); + } + if (rc < 0) { + kfree(context); + if (rc != -ENODATA) { + pr_warn("SELinux: %s: getxattr returned %d for dev=%s ino=%ld\n", + __func__, -rc, inode->i_sb->s_id, inode->i_ino); + return rc; + } + *sid = def_sid; + return 0; + } + + rc = security_context_to_sid_default(&selinux_state, context, rc, sid, + def_sid, GFP_NOFS); + if (rc) { + char *dev = inode->i_sb->s_id; + unsigned long ino = inode->i_ino; + + if (rc == -EINVAL) { + pr_notice_ratelimited("SELinux: inode=%lu on dev=%s was found to have an invalid context=%s. This indicates you may need to relabel the inode or the filesystem in question.\n", + ino, dev, context); + } else { + pr_warn("SELinux: %s: context_to_sid(%s) returned %d for dev=%s ino=%ld\n", + __func__, context, -rc, dev, ino); + } + } + kfree(context); + return 0; +} + /* The inode's security attributes must be initialized before first use. */ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dentry) { @@ -1362,9 +1425,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent u32 task_sid, sid = 0; u16 sclass; struct dentry *dentry; -#define INITCONTEXTLEN 255 - char *context = NULL; - unsigned len = 0; int rc = 0; if (isec->initialized == LABEL_INITIALIZED) @@ -1432,72 +1492,11 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent goto out; } - len = INITCONTEXTLEN; - context = kmalloc(len+1, GFP_NOFS); - if (!context) { - rc = -ENOMEM; - dput(dentry); - goto out; - } - context[len] = '\0'; - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len); - if (rc == -ERANGE) { - kfree(context); - - /* Need a larger buffer. Query for the right size. */ - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, NULL, 0); - if (rc < 0) { - dput(dentry); - goto out; - } - len = rc; - context = kmalloc(len+1, GFP_NOFS); - if (!context) { - rc = -ENOMEM; - dput(dentry); - goto out; - } - context[len] = '\0'; - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len); - } + rc = inode_doinit_use_xattr(inode, dentry, sbsec->def_sid, + &sid); dput(dentry); - if (rc < 0) { - if (rc != -ENODATA) { - pr_warn("SELinux: %s: getxattr returned " - "%d for dev=%s ino=%ld\n", __func__, - -rc, inode->i_sb->s_id, inode->i_ino); - kfree(context); - goto out; - } - /* Map ENODATA to the default file SID */ - sid = sbsec->def_sid; - rc = 0; - } else { - rc = security_context_to_sid_default(&selinux_state, - context, rc, &sid, - sbsec->def_sid, - GFP_NOFS); - if (rc) { - char *dev = inode->i_sb->s_id; - unsigned long ino = inode->i_ino; - - if (rc == -EINVAL) { - if (printk_ratelimit()) - pr_notice("SELinux: inode=%lu on dev=%s was found to have an invalid " - "context=%s. This indicates you may need to relabel the inode or the " - "filesystem in question.\n", ino, dev, context); - } else { - pr_warn("SELinux: %s: context_to_sid(%s) " - "returned %d for dev=%s ino=%ld\n", - __func__, context, -rc, dev, ino); - } - kfree(context); - /* Leave with the unlabeled SID */ - rc = 0; - break; - } - } - kfree(context); + if (rc) + goto out; break; case SECURITY_FS_USE_TASK: sid = task_sid; @@ -1548,9 +1547,21 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent goto out; rc = selinux_genfs_get_sid(dentry, sclass, sbsec->flags, &sid); - dput(dentry); - if (rc) + if (rc) { + dput(dentry); goto out; + } + + if ((sbsec->flags & SE_SBGENFS_XATTR) && + (inode->i_opflags & IOP_XATTR)) { + rc = inode_doinit_use_xattr(inode, dentry, + sid, &sid); + if (rc) { + dput(dentry); + goto out; + } + } + dput(dentry); } break; } -- cgit v1.2.3 From ec882da5cda911e799b8a5ede94d099fdc0c656b Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Fri, 22 Feb 2019 15:57:17 +0100 Subject: selinux: implement the kernfs_init_security hook The hook applies the same logic as selinux_determine_inode_label(), with the exception of the super_block handling, which will be enforced on the actual inodes later by other hooks. Signed-off-by: Ondrej Mosnacek [PM: minor merge fixes] Signed-off-by: Paul Moore --- security/selinux/hooks.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) (limited to 'security/selinux/hooks.c') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 085409b36794..ab4b049daf17 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -89,6 +89,8 @@ #include #include #include +#include +#include /* for hashlen_string() */ #include #include "avc.h" @@ -3382,6 +3384,68 @@ static int selinux_inode_copy_up_xattr(const char *name) return -EOPNOTSUPP; } +/* kernfs node operations */ + +int selinux_kernfs_init_security(struct kernfs_node *kn_dir, + struct kernfs_node *kn) +{ + const struct task_security_struct *tsec = current_security(); + u32 parent_sid, newsid, clen; + int rc; + char *context; + + rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, NULL, 0); + if (rc == -ENODATA) + return 0; + else if (rc < 0) + return rc; + + clen = (u32)rc; + context = kmalloc(clen, GFP_KERNEL); + if (!context) + return -ENOMEM; + + rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, context, + clen); + if (rc < 0) { + kfree(context); + return rc; + } + + rc = security_context_to_sid(&selinux_state, context, clen, &parent_sid, + GFP_KERNEL); + kfree(context); + if (rc) + return rc; + + if (tsec->create_sid) { + newsid = tsec->create_sid; + } else { + u16 secclass = inode_mode_to_security_class(kn->mode); + struct qstr q; + + q.name = kn->name; + q.hash_len = hashlen_string(kn_dir, kn->name); + + rc = security_transition_sid(&selinux_state, tsec->sid, + parent_sid, secclass, &q, + &newsid); + if (rc) + return rc; + } + + rc = security_sid_to_context_force(&selinux_state, newsid, + &context, &clen); + if (rc) + return rc; + + rc = kernfs_security_xattr_set(kn, XATTR_SELINUX_SUFFIX, context, clen, + XATTR_CREATE); + kfree(context); + return rc; +} + + /* file security operations */ static int selinux_revalidate_file_permission(struct file *file, int mask) @@ -6730,6 +6794,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up), LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr), + LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security), + LSM_HOOK_INIT(file_permission, selinux_file_permission), LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security), LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl), -- cgit v1.2.3 From c72c4cde8095aa0e4336cb337dac25d6e347240d Mon Sep 17 00:00:00 2001 From: YueHaibing Date: Fri, 22 Mar 2019 22:04:00 +0800 Subject: selinux: Make selinux_kernfs_init_security static Fix sparse warning: security/selinux/hooks.c:3389:5: warning: symbol 'selinux_kernfs_init_security' was not declared. Should it be static? Signed-off-by: YueHaibing Acked-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/hooks.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'security/selinux/hooks.c') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index ab4b049daf17..b6e61524d68d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3386,8 +3386,8 @@ static int selinux_inode_copy_up_xattr(const char *name) /* kernfs node operations */ -int selinux_kernfs_init_security(struct kernfs_node *kn_dir, - struct kernfs_node *kn) +static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, + struct kernfs_node *kn) { const struct task_security_struct *tsec = current_security(); u32 parent_sid, newsid, clen; -- cgit v1.2.3 From 1537ad15c9c59ce852748578eb5633139053e86b Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Wed, 3 Apr 2019 09:29:41 +0200 Subject: kernfs: fix xattr name handling in LSM helpers The implementation of kernfs_security_xattr_*() helpers reuses the kernfs_node_xattr_*() functions, which take the suffix of the xattr name and extract full xattr name from it using xattr_full_name(). However, this function relies on the fact that the suffix passed to xattr handlers from VFS is always constructed from the full name by just incerementing the pointer. This doesn't necessarily hold for the callers of kernfs_security_xattr_*(), so their usage will easily lead to out-of-bounds access. Fix this by moving the xattr name reconstruction to the VFS xattr handlers and replacing the kernfs_security_xattr_*() helpers with more general kernfs_xattr_*() helpers that take full xattr name and allow accessing all kernfs node's xattrs. Reported-by: kernel test robot Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization") Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook") Signed-off-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- fs/kernfs/inode.c | 62 ++++++++++++++++-------------------------------- include/linux/kernfs.h | 18 +++++++------- security/selinux/hooks.c | 9 ++++--- 3 files changed, 33 insertions(+), 56 deletions(-) (limited to 'security/selinux/hooks.c') diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c index 673ef598d97d..f89a0f13840e 100644 --- a/fs/kernfs/inode.c +++ b/fs/kernfs/inode.c @@ -288,63 +288,57 @@ int kernfs_iop_permission(struct inode *inode, int mask) return generic_permission(inode, mask); } -static int kernfs_node_xattr_get(const struct xattr_handler *handler, - struct kernfs_node *kn, const char *suffix, - void *value, size_t size) +int kernfs_xattr_get(struct kernfs_node *kn, const char *name, + void *value, size_t size) { - const char *name = xattr_full_name(handler, suffix); - struct kernfs_iattrs *attrs; - - attrs = kernfs_iattrs_noalloc(kn); + struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn); if (!attrs) return -ENODATA; return simple_xattr_get(&attrs->xattrs, name, value, size); } -static int kernfs_node_xattr_set(const struct xattr_handler *handler, - struct kernfs_node *kn, const char *suffix, - const void *value, size_t size, int flags) +int kernfs_xattr_set(struct kernfs_node *kn, const char *name, + const void *value, size_t size, int flags) { - const char *name = xattr_full_name(handler, suffix); - struct kernfs_iattrs *attrs; - - attrs = kernfs_iattrs(kn); + struct kernfs_iattrs *attrs = kernfs_iattrs(kn); if (!attrs) return -ENOMEM; return simple_xattr_set(&attrs->xattrs, name, value, size, flags); } -static int kernfs_xattr_get(const struct xattr_handler *handler, - struct dentry *unused, struct inode *inode, - const char *suffix, void *value, size_t size) +static int kernfs_vfs_xattr_get(const struct xattr_handler *handler, + struct dentry *unused, struct inode *inode, + const char *suffix, void *value, size_t size) { + const char *name = xattr_full_name(handler, suffix); struct kernfs_node *kn = inode->i_private; - return kernfs_node_xattr_get(handler, kn, suffix, value, size); + return kernfs_xattr_get(kn, name, value, size); } -static int kernfs_xattr_set(const struct xattr_handler *handler, - struct dentry *unused, struct inode *inode, - const char *suffix, const void *value, - size_t size, int flags) +static int kernfs_vfs_xattr_set(const struct xattr_handler *handler, + struct dentry *unused, struct inode *inode, + const char *suffix, const void *value, + size_t size, int flags) { + const char *name = xattr_full_name(handler, suffix); struct kernfs_node *kn = inode->i_private; - return kernfs_node_xattr_set(handler, kn, suffix, value, size, flags); + return kernfs_xattr_set(kn, name, value, size, flags); } static const struct xattr_handler kernfs_trusted_xattr_handler = { .prefix = XATTR_TRUSTED_PREFIX, - .get = kernfs_xattr_get, - .set = kernfs_xattr_set, + .get = kernfs_vfs_xattr_get, + .set = kernfs_vfs_xattr_set, }; static const struct xattr_handler kernfs_security_xattr_handler = { .prefix = XATTR_SECURITY_PREFIX, - .get = kernfs_xattr_get, - .set = kernfs_xattr_set, + .get = kernfs_vfs_xattr_get, + .set = kernfs_vfs_xattr_set, }; const struct xattr_handler *kernfs_xattr_handlers[] = { @@ -352,17 +346,3 @@ const struct xattr_handler *kernfs_xattr_handlers[] = { &kernfs_security_xattr_handler, NULL }; - -int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix, - void *value, size_t size) -{ - return kernfs_node_xattr_get(&kernfs_security_xattr_handler, - kn, suffix, value, size); -} - -int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix, - void *value, size_t size, int flags) -{ - return kernfs_node_xattr_set(&kernfs_security_xattr_handler, - kn, suffix, value, size, flags); -} diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h index 39eea07c2900..7987e0f89b69 100644 --- a/include/linux/kernfs.h +++ b/include/linux/kernfs.h @@ -371,10 +371,10 @@ __poll_t kernfs_generic_poll(struct kernfs_open_file *of, struct poll_table_struct *pt); void kernfs_notify(struct kernfs_node *kn); -int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix, - void *value, size_t size); -int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix, - void *value, size_t size, int flags); +int kernfs_xattr_get(struct kernfs_node *kn, const char *name, + void *value, size_t size); +int kernfs_xattr_set(struct kernfs_node *kn, const char *name, + const void *value, size_t size, int flags); const void *kernfs_super_ns(struct super_block *sb); int kernfs_get_tree(struct fs_context *fc); @@ -478,14 +478,12 @@ static inline int kernfs_setattr(struct kernfs_node *kn, static inline void kernfs_notify(struct kernfs_node *kn) { } -static inline int kernfs_security_xattr_get(struct kernfs_node *kn, - const char *suffix, void *value, - size_t size) +static inline int kernfs_xattr_get(struct kernfs_node *kn, const char *name, + void *value, size_t size) { return -ENOSYS; } -static inline int kernfs_security_xattr_set(struct kernfs_node *kn, - const char *suffix, void *value, - size_t size, int flags) +static inline int kernfs_xattr_set(struct kernfs_node *kn, const char *name, + const void *value, size_t size, int flags) { return -ENOSYS; } static inline const void *kernfs_super_ns(struct super_block *sb) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index b6e61524d68d..d5fdcb0d26fe 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3394,7 +3394,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, int rc; char *context; - rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, NULL, 0); + rc = kernfs_xattr_get(kn_dir, XATTR_NAME_SELINUX, NULL, 0); if (rc == -ENODATA) return 0; else if (rc < 0) @@ -3405,8 +3405,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, if (!context) return -ENOMEM; - rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, context, - clen); + rc = kernfs_xattr_get(kn_dir, XATTR_NAME_SELINUX, context, clen); if (rc < 0) { kfree(context); return rc; @@ -3439,8 +3438,8 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, if (rc) return rc; - rc = kernfs_security_xattr_set(kn, XATTR_SELINUX_SUFFIX, context, clen, - XATTR_CREATE); + rc = kernfs_xattr_set(kn, XATTR_NAME_SELINUX, context, clen, + XATTR_CREATE); kfree(context); return rc; } -- cgit v1.2.3 From c750e6929d3c76d13d1d0ba475989d6dd74785d5 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa Date: Fri, 12 Apr 2019 19:59:34 +0900 Subject: selinux: Check address length before reading address family KMSAN will complain if valid address length passed to bind()/connect() is shorter than sizeof("struct sockaddr"->sa_family) bytes. Signed-off-by: Tetsuo Handa Signed-off-by: Paul Moore --- security/selinux/hooks.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'security/selinux/hooks.c') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d5fdcb0d26fe..c61787b15f27 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4512,7 +4512,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in struct lsm_network_audit net = {0,}; struct sockaddr_in *addr4 = NULL; struct sockaddr_in6 *addr6 = NULL; - u16 family_sa = address->sa_family; + u16 family_sa; unsigned short snum; u32 sid, node_perm; @@ -4522,6 +4522,9 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in * need to check address->sa_family as it is possible to have * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET. */ + if (addrlen < offsetofend(struct sockaddr, sa_family)) + return -EINVAL; + family_sa = address->sa_family; switch (family_sa) { case AF_UNSPEC: case AF_INET: @@ -4654,6 +4657,8 @@ static int selinux_socket_connect_helper(struct socket *sock, * need to check address->sa_family as it is possible to have * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET. */ + if (addrlen < offsetofend(struct sockaddr, sa_family)) + return -EINVAL; switch (address->sa_family) { case AF_INET: addr4 = (struct sockaddr_in *)address; -- cgit v1.2.3