From: Al Viro <viro@ZenIV.linux.org.uk>
To: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-security-module@vger.kernel.org
Subject: new_inode_smack() bogosities
Date: Wed, 4 Jan 2012 22:13:35 +0000 [thread overview]
Message-ID: <20120104221335.GF23916@ZenIV.linux.org.uk> (raw)
1) in smack_sb_kern_mount()
isp = inode->i_security;
if (isp == NULL)
inode->i_security = new_inode_smack(sp->smk_root);
else
isp->smk_inode = sp->smk_root;
looks very fishy. How in hell had that inode managed to get created
in the first place without going through smack_inode_alloc_security()?
Is that about mounting an fs instance with in-core superblock that
is older than the call of smack_init()? That shouldn't be possible due
to initcall ordering, but if that's the case, just what would happen when
we step on *other* inodes on the same fs? There's a bunch of places where
smack assumes that ->i_security is never NULL... And if that's really
impossible, WTF is that new_inode_smack() doing there?
2) again in smack_sb_kern_mount()
spin_lock(&sp->smk_sblock);
if (sp->smk_initialized != 0) {
spin_unlock(&sp->smk_sblock);
return 0;
}
sp->smk_initialized = 1;
spin_unlock(&sp->smk_sblock);
For one thing, security_sb_kern_mount() is serialized by sb->s_umount.
For another, if it wouldn't be... this code wouldn't be able to prevent
a race. Sure, only one of them will proceed to initialize the sucker.
And another may cheerfully return before the work is actually done...
In any case, it *is* serialized on per-superblock basis.
3) in smk_fill_super(), we have
root_inode = sb->s_root->d_inode;
root_inode->i_security = new_inode_smack(smack_known_floor.smk_known);
Again, huh? This should be called after smack_init() had been done; hell,
you explicitly say so in the comments:
* Do not register smackfs if Smack wasn't enabled
* on boot. We can not put this method normally under the
* smack_init() code path since the security subsystem get
* initialized before the vfs caches.
Sounds like a misspelled assignment to ->smk_inode to me (and a leak, while
we are at it)...
reply other threads:[~2012-01-04 22:13 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
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=20120104221335.GF23916@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=casey@schaufler-ca.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.