All of lore.kernel.org
 help / color / mirror / Atom feed
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.