ubifs assert when creating a SMACK transmuting directory.

* ubifs assert when creating a SMACK transmuting directory.
@ 2019-07-22 21:26 ` Martin Townsend
  0 siblings, 0 replies; 2+ messages in thread
From: Martin Townsend @ 2019-07-22 21:26 UTC (permalink / raw
  To: linux-mtd, linux-integrity

Hi,

The following backtrack was reported by our dev team.

[<8010ea60>] (unwind_backtrace) from [<8010c4d8>] (show_stack+0x10/0x14)
[<8010c4d8>] (show_stack) from [<803b590c>] (ubifs_xattr_set+0x210/0x3ec)
[<803b590c>] (ubifs_xattr_set) from [<80272134>] (__vfs_setxattr+0x70/0x80)
[<80272134>] (__vfs_setxattr) from [<803ecd8c>]
(smack_d_instantiate+0x41c/0x430)
[<803ecd8c>] (smack_d_instantiate) from [<803e4ef0>]
(security_d_instantiate+0x34/0x54)
[<803e4ef0>] (security_d_instantiate) from [<80263edc>]
(d_instantiate+0x28/0x4c)
[<80263edc>] (d_instantiate) from [<80392858>] (ubifs_mkdir+0x1f8/0x200)
[<80392858>] (ubifs_mkdir) from [<80257124>] (vfs_mkdir+0x148/0x1dc)
[<80257124>] (vfs_mkdir) from [<8025add8>] (SyS_mkdirat+0x84/0xec)
[<8025add8>] (SyS_mkdirat) from [<80107dfc>] (__sys_trace_return+0x0/0x10)

Looking at the code for smack_d_instantiate it's going down the following path
/*
 * Transmuting directory
 */
if (S_ISDIR(inode->i_mode)) {
        /*
         * 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 = __vfs_setxattr(dp, inode,
                        XATTR_NAME_SMACKTRANSMUTE,
                        TRANS_TRUE, TRANS_TRUE_SIZE,
                        0);
        } else {
which ends up in ubifs_xattr_set which is expecting the inode
semaphore to be locked and hence the assert message.

I can reproduce this by creating a directory with SMACK64TRANSMUTE and
then adding a rule with 't' set and then creating a directory

mkdir test
chmod 777 test
chsmack -a update test
chsmack -t test

echo programmingapp > /proc/self/attr/current
echo programmingapp update rwxat > /sys/fs/smackfs/load2

Creating a directory in test which trigger the ubifs assert.

Looking at the code some more, don't we need to lock the inode when
calling __vfs_setxattr above as the inode will have already been
created (ubifs_init_security in ubifs_mkdir) and added to the inode
hash and another process could potentially set an extended attribute
on the directory whilst we are trying to add the transmute attribute
here?

I created a patch which put an inode_trylock around __vfs_setxattr and
the ubifs assert has now disappeared in the use case described.
Although this fixes things for a UBIFS I had a quick look at other
filesystems to see how they handle setting extended attributes and the
2 I looked at have a i_xattr_sem which is in some wrapper structure
around an inode so maybe this isn't the best solution but maybe the
inode_trylock should be moved into the ubifs_xattr_set function
instead of the

if (check_lock)
        ubifs_assert(c, inode_is_locked(host));

to be more in line with what the other filesystems do.  This is on a
4.9 vendor kernel for an i.MX system but looking at 5.2 I think the
problem would still exist but not entirely sure.  Either way is
replacing the above check with inode_trylock a valid solution or is
there a better way of solving this?

Many Thanks,
Martin.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 2+ messages in thread