On Thu, Dec 22, 2016 at 7:54 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Dec 22, 2016 at 12:56 AM, Theodore Ts'o <tytso@xxxxxxx> wrote:
...
>>> Note that all those file system copy&pasted a potential bug.
>>>
>>> This is an array of size 15:
>>>
>>> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
>>>
>>> and it is always addressed this way:
>>>
>>> ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT];
>>>
>>> Which *can* try to address ext2_type_by_mode[15]
>>>
>>> Now, can you say for certain that you can never get a malformed i_mode
>>> with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from
>>> some code calling into VFS functions, which do not sanity check the
>>> file type of the mode argument.
>>
>> It's not a problem and not a bug, because we only assign the file type
>> when the inode is created, and at that point, the i_mode is set by the
>> *kernel*, and not by the on-disk encoding.
>>
>
> Not entirley true.
> On rename some fs set dentry type of target by converting the mode of
> the renamed object.
> I don't remember what ext4 does, but I think it does the same,
> so malformed imode on disk could potentially get you there.
>
So I checked the code.
ext4 is off the hook because it sanitizes mode in ext4_iget and
returns -ECORRUPTEDFS
e2fsprogs' ext2_file_type() is off the hook as well, not using a
conversion table at all.
ext2 on the other hand just calls init_special_inode() for the else case
which in turn print a debug message "init_special_inode: bogus i_mode (%o)"
and doesn't do anything about it, so a later rename can certainly try to convert
a bogus i_mode of S_IFMT and escape the boundaries of the conversion table.
Anyway, I won't try to force feed anyone fix patches.
I am going to re-send the xfs patch as an independent fix patch and will
send a matching patch to xfsprogs.
Any other fs maintainers that want the fix are welcome to apply their
own version
or ask me to send a fix their way.
Cheers,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
