From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-oi0-f65.google.com ([209.85.218.65]:32863 "EHLO
        mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1758123AbcLVUa7 (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Thu, 22 Dec 2016 15:30:59 -0500
Received: by mail-oi0-f65.google.com with SMTP id f201so34043302oib.0
        for <linux-fsdevel@vger.kernel.org>; Thu, 22 Dec 2016 12:30:59 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <CAOQ4uxh60bG+jg-Ukp_0vdGX_C8tSRAGNu6E7_tdOq_9fQXZ2Q@mail.gmail.com>
References: <1482178268-22883-1-git-send-email-amir73il@gmail.com>
 <1482178268-22883-12-git-send-email-amir73il@gmail.com> <20161220002832.GB7297@birch.djwong.org>
 <CAOQ4uxj3hpuuADiTv0q6BBvhUvmpi=2mkVdk=1pXO2cCdT05Qg@mail.gmail.com>
 <20161221052317.GA4758@dastard> <20161221150619.hkgsqwcdzkqlum4v@thunk.org>
 <CAOQ4uxjnUNQWiwx_rExePs_S_eTZWbVkqY1UgBUzbehKn4LxWQ@mail.gmail.com>
 <20161221225605.qcgpwkyrugwmjnza@thunk.org> <CAOQ4uxh60bG+jg-Ukp_0vdGX_C8tSRAGNu6E7_tdOq_9fQXZ2Q@mail.gmail.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, 22 Dec 2016 22:30:57 +0200
Message-ID: <CAOQ4uxjfYXSsN_S3JES23r2RPCU90xJjfDtn6ex2ujdjomjtmg@mail.gmail.com>
Subject: Re: [RFC][PATCH 11/11] xfs: use common file type conversion
To: "Theodore Ts'o" <tytso@mit.edu>, Jan Kara <jack@suse.com>,
        "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Dave Chinner <david@fromorbit.com>, Chris Mason <clm@fb.com>,
        Boaz Harrosh <boaz@plexistor.com>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>,
        Mark Fasheh <mfasheh@versity.com>,
        Evgeniy Dushistov <dushistov@mail.ru>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Al Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Thu, Dec 22, 2016 at 7:54 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Dec 22, 2016 at 12:56 AM, Theodore Ts'o <tytso@mit.edu> 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.