Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

Hello,

After commit, "b968091 security_d_instantiate(): move to the point prior to attaching dentry to inode", booting on system with systemd and security smack, following kernel panic occurs.

---

Unable to handle kernel paging request at virtual address fffffff4
pgd = eda74000
[fffffff4] *pgd=6fffd861, *pte=00000000, *ppte=00000000
Internal error: Oops: 37 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: systemd Not tainted 4.6.0-11010-gdc03c0f-dirty #54
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
task: ee948000 ti: ee942000 task.ti: ee942000
PC is at do_raw_spin_lock+0x14/0x1c0
LR is at _raw_spin_lock+0x28/0x2c
pc : [<c016e69c>]    lr : [<c0a9f608>]    psr: 000f0013

sp : ee943d98 ip : ee943dc0 fp : ee943dbc
r10: 00000000 r9 : ed8a1f80 r8 : fffffff0
r7 : eea57d40 r6 : c0d5c764 r5 : ffffffe8 r4 : fffffff0 r3 : ee948000 r2 : 00000001 r1 : c0d5c77e r0 : fffffff0
Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 6da7406a DAC: 00000051 Process systemd (pid: 1, stack limit = 0xee942210)

Stack: (0xee943d98 to 0xee944000)

3d80: fffffff0 ffffffe8
3da0: c0d5c764 eea57d40 fffffff0 ed8a1f80 ee943dd4 ee943dc0 c0a9f608 c016e694 3dc0: 00000004 00000000 ee943dfc ee943dd8 c0271bf8 c0a9f5ec 00000000 c0d5c7ec 3de0: 00000000 ed5f9428 ee314480 ed8a1f80 ee943e24 ee943e00 c0202e40 c0271ba4 3e00: 00000000 c0270d28 00000004 ed5f9428 c0d5c7ec 00000004 ee943e54 ee943e28 3e20: c0270d54 c0202e08 00000004 00000000 00000100 c0d5c76d eda8d380 eda8d380 3e40: eea51430 eda8d38c ee943e9c ee943e58 c03aa4c0 c0270cec 00000000 ed5f9440 3e60: c114a3c8 00000004 ee943edc ee943e78 c03a4e60 c114ad70 c114a678 eea51430 3e80: ed5f9428 00000000 ffffff9c 00000000 ee943ebc ee943ea0 c03a4d84 c03aa1c4 3ea0: eea51430 ed5f9428 eea51430 ed5f9428 ee943edc ee943ec0 c0262c18 c03a4d4c 3ec0: eea507d0 00000000 eea50780 eea51430 ee943f1c ee943ee0 c020363c c0262bf8 3ee0: 00000000 c01014c4 386d439a 00000000 35a4e900 c02038cc 000001ed 000001ed 3f00: eea50780 ed5f9428 00000000 7f65f7f8 ee943f34 ee943f20 c02038f0 c0203568 3f20: 000001ed eea50780 ee943f64 ee943f38 c0255fbc c02038d8 ee3a4015 ffffffff 3f40: 000001ed 000001ed ed5f9428 7f65f7f8 00000002 000001ed ee943f94 ee943f68 3f60: c025a1ec c0255f08 eda96310 ed5f8d10 000001ed 7f65f7f8 003520a2 00000027 3f80: c0109248 ee942000 ee943fa4 ee943f98 c025a25c c025a180 00000000 ee943fa8 3fa0: c0109040 c025a244 000001ed 7f65f7f8 7f65f7f8 000001ed 00000000 00104000 3fc0: 000001ed 7f65f7f8 003520a2 00000027 b6f9beb0 7f6bb9b0 bebe3998 bebe3988 3fe0: 7f6bb9e8 bebe387c 7f60419d b6df434c 600f0010 7f65f7f8 ffffffff fffffffb [<c016e69c>] (do_raw_spin_lock) from [<c0a9f608>] (_raw_spin_lock+0x28/0x2c) [<c0a9f608>] (_raw_spin_lock) from [<c0271bf8>] (simple_xattr_set+0x60/0x158) [<c0271bf8>] (simple_xattr_set) from [<c0202e40>] (shmem_xattr_handler_set+0x44/0x4c) [<c0202e40>] (shmem_xattr_handler_set) from [<c0270d54>] (generic_setxattr+0x74/0x7c) [<c0270d54>] (generic_setxattr) from [<c03aa4c0>] (smack_d_instantiate+0x308/0x390) [<c03aa4c0>] (smack_d_instantiate) from [<c03a4d84>] (security_d_instantiate+0x44/0x64) [<c03a4d84>] (security_d_instantiate) from [<c0262c18>] (d_instantiate+0x2c/0x5c) [<c0262c18>] (d_instantiate) from [<c020363c>] (shmem_mknod+0xe0/0xfc) [<c020363c>] (shmem_mknod) from [<c02038f0>] (shmem_mkdir+0x24/0x3c) [<c02038f0>] (shmem_mkdir) from [<c0255fbc>] (vfs_mkdir+0xc0/0x10c) [<c0255fbc>] (vfs_mkdir) from [<c025a1ec>] (SyS_mkdirat+0x78/0xc4) [<c025a1ec>] (SyS_mkdirat) from [<c025a25c>] (SyS_mkdir+0x24/0x28) [<c025a25c>] (SyS_mkdir) from [<c0109040>] (ret_fast_syscall+0x0/0x3c)

Code: e92ddbf0 e24cb004 e52de004 e8bd4000 (e5902004) 
---[ end trace ec9873d14ae12b14 ]---

It works fine if reverting the commit, "b968091 security_d_instantiate(): move to the point prior to attaching dentry to inode", for d_instantiate() like following.

---

@@ -1793,11 +1793,11 @@ void d_instantiate(struct dentry *entry, struct inode * inode)
 {
        BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
        if (inode) {
-               security_d_instantiate(entry, inode);
                spin_lock(&inode->i_lock);
                __d_instantiate(entry, inode);
                spin_unlock(&inode->i_lock);
        }
+       security_d_instantiate(entry, inode);
 }
 EXPORT_SYMBOL(d_instantiate);
 

---

In my test environment, following related configs are enabled.

CONFIG_SHMEM=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_TMPFS_XATTR=y

CONFIG_SECURITY_SMACK=y
CONFIG_DEFAULT_SECURITY_SMACK=y
CONFIG_DEFAULT_SECURITY="smack"

Best Regards,
- Seung-Woo Kim

                        /*
                         * If this is a new directory and the label was
                         * transmuted when the inode was initialized
                         * set the transmute attribute on the directory
                         * and mark the inode.
                         *
                         * If there is a transmute attribute on the
                         * directory mark the inode.
                         */
                        if (isp->smk_flags & SMK_INODE_CHANGED) {
                                isp->smk_flags &= ~SMK_INODE_CHANGED;
                                rc = inode->i_op->setxattr(dp,
                                        XATTR_NAME_SMACKTRANSMUTE,
                                        TRANS_TRUE, TRANS_TRUE_SIZE,
                                        0);

Damnation ;-/ That change (separating inode and dentry arguments of ->getxattr() so that security_d_instantiate() could be called before dentry is hashed or attached to inode) had been discussed back in early March and reaction of Casey back then had been basically "I believe that smack can live with that, will verify that in about a week". With no followup objections - neither immediate, nor in a week. As the matter of fact, your posting is the first time anyone has reported stepping into that problem. And that change had been present in linux-next since the beginning of May ;-/ Sigh...

Can't be reverted in mainline. Not without shitloads of other stuff.

There is a fairly straightforward way to handle that - do to ->setxattr() what we'd already done to ->getxattr(). See vfs.git#smack-fix. Warning:
it's only build-tested. I'm going to have it go through LTP and xfstests shortly; _please_ check if it works on your setup, because I've no idea how to put together a testing setup for smack.

FWIW, that couple of commits seems to survive the testing here and is pretty obvious. I have _NOT_ tested it on smack setups, so I really want somebody (Casey or someone in Samsung) to check if it fixes the problem. The change itself isn't tricky, but I fucking _hate_ doing that this late in the merge window ;-/

Al, if you want Casey to help test, I think you should write out the full git repository address, rather than just say "See vfs.git#smack-fix".

Anybody who isn't used to pulling for you will just wonder where you keep your tree. And even I, who _am_ used to pulling from you, would have to look it up, so it's a lot more convenient if you actually write out the whole thing,

Casey, Al is talking about

git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs smack-fix

and Al, please make your commit messages more informative than just "switch ->setxattr() to passing dentry and inode separately". You can see that from the patch. Please add a _why_ something is done, not just what it does.

Linus

Point taken.

Umm... Would something along the lines of

switch ->setxattr() to passing inode and dentry separately

smack ->d_instantiate() uses ->setxattr(), so to be able to call it before we'd hashed the new dentry and attached it to inode, we ->setxattr() instances get the inode as an explicit argument rather than obtaining it from dentry. Similar change for ->getxattr() had been done commit ce23e64. Unlike ->getxattr() (which is used both by selinux and smack instances of ->d_instantiate()) ->setxattr() is used only by smack one and unfortunately it had been missed back then.

be detailed enough for the second one with

switch xattr_handler->set() to passing inode and dentry separately

preparation for similar switch in ->setxattr() (see the next commit for rationale)

for the first one?

Sure. That makes me go "ahh, ok, I see why".

Linus

Amended and force-pushed...

Ok, I'll ignore that branch for now, in the hopes that there will be actual testing success. Please send a full pull request at that point, ok?

Linus

Sure. FWIW, the original bug report had been from +0900, so it's what, about 5am Saturday morning there? Let's hope somebody will be there today, weekend or not...

Al, I have verified that upstream has the problem and that your vfs.git smack-fix branch works correctly.

Hello,

It seems a bit late, but it works without reported panic for my environment too.

Thanks,
- Seung-Woo Kim

Seung-Woo Kim
Samsung Software R&D Center
--

I haven't actually seen the problem, but I've been having real trouble getting a systemd configuration working properly. The quickest validation will probably be coming from Seung-Woo Kim, who reported the issue initially. I am working to verify both the problem and the fix.

To trigger it you need to end up in smack_d_instantiate() for a directory that had SMK_INODE_CHANGED set in smack_inode_init_security(). IOW, smk_inode_transmutable() being true for its parent and smk_access_entry() for that parent returning something with MAY_TRANSMUTE in it.

I'm not familiar enough with smack guts to put together a reproducer, but *ANY* call of ->setxattr() from smack_d_instantiate() on xattr-supporting filesystem will blow up in the mainline. At that point dentry still has NULL ->d_inode, so ->setxattr() instances are going to oops as soon as they try to do anything with the inode. All it takes is getting to that method call.