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

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

[Seung-Woo Kim]

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.

---
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -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

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Al Viro]

On Fri, May 27, 2016 at 08:09:09PM +0900, Seung-Woo Kim wrote:

> 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.

                        /*
                         * 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...

> 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.

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.

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Al Viro]

On Fri, May 27, 2016 at 04:11:41PM +0100, Al Viro wrote:

> > 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.
> 
>                         /*
>                          * 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...
> 
> > 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.
> 
> 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 ;-/

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Linus Torvalds]

On Fri, May 27, 2016 at 11:51 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, May 27, 2016 at 04:11:41PM +0100, Al Viro wrote:
>> 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

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Casey Schaufler]

On 5/27/2016 11:51 AM, Al Viro wrote:
> On Fri, May 27, 2016 at 04:11:41PM +0100, Al Viro wrote:
>
>>> 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.
>>                         /*
>>                          * 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...
>>
>>> 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.
>> 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 ;-/

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.

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Al Viro]

On Fri, May 27, 2016 at 12:01:05PM -0700, Linus Torvalds wrote:

> 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,

Point taken.

> 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.

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?

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Linus Torvalds]

On Fri, May 27, 2016 at 12:26 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Umm...  Would something along the lines of [...]

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

             Linus

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Al Viro]

On Fri, May 27, 2016 at 12:03:37PM -0700, Casey Schaufler wrote:

> 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.

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Al Viro]

On Fri, May 27, 2016 at 12:34:34PM -0700, Linus Torvalds wrote:
> On Fri, May 27, 2016 at 12:26 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Umm...  Would something along the lines of [...]
> 
> Sure. That makes me go "ahh, ok, I see why".

Amended and force-pushed...

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Linus Torvalds]

On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> 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

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Al Viro]

On Fri, May 27, 2016 at 12:48:23PM -0700, Linus Torvalds wrote:
> On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > 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?

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...

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Casey Schaufler]

On 5/27/2016 1:24 PM, Al Viro wrote:
> On Fri, May 27, 2016 at 12:48:23PM -0700, Linus Torvalds wrote:
>> On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>> 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?
> 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.

Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

[Seung-Woo Kim]

Hello,

On 2016년 05월 28일 07:44, Casey Schaufler wrote:
> On 5/27/2016 1:24 PM, Al Viro wrote:
>> On Fri, May 27, 2016 at 12:48:23PM -0700, Linus Torvalds wrote:
>>> On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>> 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?
>> 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.
> 

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
--