On 2022/09/20 9:29, Seth Forshee wrote:
>>>>> Odd that we didn't get any of the reports. Thanks for relying this.
>>>>> I'll massage this a tiny bit, apply and will test with syzbot.
Since KMSAN is not yet upstreamed, uninit-value reports can come only after bugs
reached upstream kernels. Also, only LSMs which implement security_path_chown()
hook and checks uid/gid values (i.e. TOMOYO) would catch this bug.
>>>>
>>>> Fyi, Seth.
>>>
>>> Because the modules are ignoring ia_valid & ATTR_XID?
>>
>> security_path_chown() takes ids as arguments, not struct iattr.
>>
>> int security_path_chown(const struct path *path, kuid_t uid, kgid_t gid)
>>
>> The ones being passed are now taken from iattr and thus potentially not
>> initialized. Even if we change it to only call security_path_chown()
>> when one of ATTR_{U,G}ID is set the other might not be set, so
>> initializing iattr.ia_vfs{u,g}id makes sense to me and should match the
>> old behavior of passing invalid ids in this situation.
>>
>> What I don't understand is why security_path_chown() is even necessary
>> when we also have security_inode_setattr(), which also gets called
>> during chown and gets the full iattr struct. Maybe there's a good
>> reason, but at first glance it seems like it could do any checks that
>> security_path_chown() is doing.
>
> Maybe the important difference is that one takes the path as an argument
> and the other only takes the dentry? I guess that might be it, though it
> still feels kind of redundant.
security_path_*() hooks are used by LSMs which check absolute pathnames.
Thus, these hooks are not redundant.
