From: Stephen Smalley <sds@tycho.nsa.gov>
To: Ondrej Mosnacek <omosnace@redhat.com>,
selinux@vger.kernel.org, Paul Moore <paul@paul-moore.com>
Cc: linux-security-module@vger.kernel.org,
Casey Schaufler <casey@schaufler-ca.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Tejun Heo <tj@kernel.org>,
linux-fsdevel@vger.kernel.org, cgroups@vger.kernel.org
Subject: Re: [PATCH v6 1/5] selinux: try security xattr after genfs for kernfs filesystems
Date: Thu, 14 Feb 2019 15:49:25 -0500 [thread overview]
Message-ID: <8569202a-1a46-2a4d-3943-01b9bc918524@tycho.nsa.gov> (raw)
In-Reply-To: <20190214095015.16032-2-omosnace@redhat.com>
On 2/14/19 4:50 AM, Ondrej Mosnacek wrote:
> 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 <omosnace@redhat.com>
> ---
> security/selinux/hooks.c | 160 +++++++++++++++-------------
> security/selinux/include/security.h | 1 +
> 2 files changed, 88 insertions(+), 73 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 81e012c66d95..7dea5b1a89a3 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -793,11 +793,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) {
> /*
> @@ -1392,6 +1394,71 @@ 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 = NULL;
> + unsigned int len = 0;
No need to initialize here since no uses or gotos prior to first assignment?
> + int rc;
> +
> + *sid = def_sid;
> +
> + if (!(inode->i_opflags & IOP_XATTR))
> + return 0;
Is this a change in behavior from before the patch? Would we have
previously called __vfs_getxattr -> xattr_resolve_name and returned
either -EIO (is_bad_inode) or -EOPNOTSUPP to the caller? Perhaps it is
fine to return 0 with the default SID here, but wanted to check.
> +
> + 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;
> + }
> + 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)
> {
> @@ -1400,9 +1467,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)
> @@ -1470,72 +1534,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;
> @@ -1586,9 +1589,20 @@ 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) {
> + rc = inode_doinit_use_xattr(inode, dentry,
> + sid, &sid);
> + if (rc) {
> + dput(dentry);
> + goto out;
> + }
> + }
> + dput(dentry);
> }
> break;
> }
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index f68fb25b5702..6e5928f951da 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -58,6 +58,7 @@
> #define SE_SBINITIALIZED 0x0100
> #define SE_SBPROC 0x0200
> #define SE_SBGENFS 0x0400
> +#define SE_SBGENFS_XATTR 0x0800
>
> #define CONTEXT_STR "context="
> #define FSCONTEXT_STR "fscontext="
>
next prev parent reply other threads:[~2019-02-14 20:49 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-14 9:50 [PATCH v6 0/5] Allow initializing the kernfs node's secctx based on its parent Ondrej Mosnacek
2019-02-14 9:50 ` [PATCH v6 1/5] selinux: try security xattr after genfs for kernfs filesystems Ondrej Mosnacek
2019-02-14 20:49 ` Stephen Smalley [this message]
2019-02-15 15:48 ` Ondrej Mosnacek
2019-02-14 9:50 ` [PATCH v6 2/5] kernfs: use simple_xattrs for security attributes Ondrej Mosnacek
2019-02-14 9:50 ` [PATCH v6 3/5] LSM: add new hook for kernfs node initialization Ondrej Mosnacek
2019-02-14 9:50 ` [PATCH v6 4/5] selinux: implement the kernfs_init_security hook Ondrej Mosnacek
2019-02-14 9:50 ` [PATCH v6 5/5] kernfs: initialize security of newly created nodes Ondrej Mosnacek
2019-02-14 15:48 ` Tejun Heo
2019-02-15 15:45 ` Ondrej Mosnacek
2019-02-15 15:50 ` Tejun Heo
2019-02-18 10:03 ` Ondrej Mosnacek
2019-02-18 21:02 ` Tejun Heo
2019-02-19 0:28 ` Casey Schaufler
2019-02-19 14:10 ` Ondrej Mosnacek
2019-02-19 14:21 ` Tejun Heo
2019-02-19 16:43 ` Casey Schaufler
2019-02-21 9:13 ` Ondrej Mosnacek
2019-02-21 16:52 ` Casey Schaufler
2019-02-22 12:52 ` Ondrej Mosnacek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8569202a-1a46-2a4d-3943-01b9bc918524@tycho.nsa.gov \
--to=sds@tycho.nsa.gov \
--cc=casey@schaufler-ca.com \
--cc=cgroups@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--cc=selinux@vger.kernel.org \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).