UBIFS assert when create new sub-dir under a transmute enabled dir with Smack enabled

[casey@xxxxxxxxxxxxxxxx (Casey Schaufler)]

On 7/3/2018 12:22 AM, Richard Weinberger wrote:
> Am Dienstag, 3. Juli 2018, 08:05:47 CEST schrieb xiaolei li:
>> Hi, Richard,
>>
>> On Sun, 2018-07-01 at 10:23 +0200, Richard Weinberger wrote:
>>> Xiaolei Li,
>>>
>>> Am Donnerstag, 21. Juni 2018, 11:04:36 CEST schrieb xiaolei li:
>>>> Hi Richard,
>>>>
>>>> I ever committed one patch[1] to fix ubifs assert in ubifs_xattr_set()
>>>> if Smack is enabled.
>>> Ohh, that patch was missed. Sorry for that.
>>>
>>>> It almost always runs OK except "creating new sub-dir under a transmute
>>>> enabled dir" which is found by Mr.Leqiao Peng. For this special case,
>>>> there still has ubifs assert problem.
>>>>
>>>> With patch[1], this problem can be reproduced by these steps ( assume
>>>> that the Smack has been enabled and the Smack userspace tool also
>>>> installed ):
>>>> 1). Add new smack rule in which the "aaa" for process object, "bbb" for
>>>> file or directory object.  "xwt" for the permission "execute, write and
>>>> transmute"
>>>>
>>>> echo "aaa bbb xwt" | smackload
>>>>
>>>> 2). Apply the label "aaa" to current sh process.   Verify the effect by
>>>> "ps -Z" 
>>>>
>>>> echo aaa > /proc/self/attr/current
>>>>
>>>> 3) Create a directory "test_dir" and assign the "bbb" label and enable
>>>> the transmute feature upon the new directory. 
>>>> Verify this by "chsmack test_dir". 
>>>>
>>>> mkdir test_dir
>>>> chsmack -a "bbb" -t test_dir
>>>>
>>>> 4) Create a new directory "newDir" under the "test_dir". 
>>>>
>>>> mkdir test_dir/newDir
>>>>
>>>>
>>>> Then, ubifs assert happens, and the stack trace is:
>>>>
>>>> UBIFS assert failed in __ubifs_setxattr at 284 (pid 4041)
>>>> CPU: 2 PID: 4041 Comm: mkdir Tainted: G S      W       4.9.90 #1
>>>> Call trace:
>>>> [<ffffff8008089ff8>] dump_backtrace+0x0/0x1d8
>>>> [<ffffff800808a254>] show_stack+0x24/0x30
>>>> [<ffffff800845f074>] dump_stack+0x94/0xb8
>>>> [<ffffff80083cdd78>] __ubifs_setxattr+0x358/0x5f8
>>>> [<ffffff80083ce384>] ubifs_xattr_set+0x64/0x3d0
>>>> [<ffffff800826c3bc>] __vfs_setxattr+0x7c/0x98
>>>> [<ffffff8008416d64>] smack_d_instantiate+0x304/0x350
>>>> [<ffffff800840cdec>] security_d_instantiate+0x4c/0x78
>>>> [<ffffff800825ca1c>] d_instantiate+0x3c/0x70
>>>> [<ffffff80083ab158>] ubifs_mkdir+0x1d8/0x1e0
>>>> [<ffffff800824f140>] vfs_mkdir2+0x128/0x1b0
>>>> [<ffffff8008254738>] SyS_mkdirat+0x80/0xf0
>>>> [<ffffff800808378c>] __sys_trace_return+0x0/0x4
>>>>
>>>> Mr. Schaufler (original developer of SMACK) has analyzed the possible
>>>> root cause:
>>>> "I have confirmed this behavior. The issue is that the inode isn't
>>>> locked during inode instantiation. It doesn't need to be, because until
>>>> instatiation is complete the inode doesn't really exist and isn't
>>>> accessible to any other task. The assertion in the ubifs code is not
>>>> appropriate. I suggest that you contact the ubifs maintainers.
>>>> Please CC me, and I will answer any questions they may have. Thank you
>>>> for the report."
>>> Yeah, this is the same case as with encrypted files. The assert makes
>>> no sense here. Thanks for reporting!
>>>
>>> Does your patch from 2017 still apply and work?
>> My patch is applied, but can not resolve the problem here.
>>
>> The root cause is that the inode isn't locked during inode instantiation
>> (from Mr.Schaufler), but we check whether inode is locked in
>> xattr_set.

There is no need to do this check as all that
gets done with the decision is to print the warning.
The problem is the assumption that the inode is locked,
which is doesn't have to be during instantiation.

> Care to send a massaged patch for this?
>
> Thanks,
> //richard
>