Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup_fast
To:     Al Viro <viro@zeniv.linux.org.uk>, linux-fsdevel@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, wugyuan@cn.ibm.com,
        jlayton@kernel.org, hsiangkao@aol.com, Jan Kara <jack@suse.cz>
References: <20190927044243.18856-1-riteshh@linux.ibm.com>
 <20191015040730.6A84742047@d06av24.portsmouth.uk.ibm.com>
From:   Ritesh Harjani <riteshh@linux.ibm.com>
Date:   Tue, 22 Oct 2019 19:08:54 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <20191015040730.6A84742047@d06av24.portsmouth.uk.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Message-Id: <20191022133855.B1B4752050@d06av21.portsmouth.uk.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

I think we have still not taken this patch. Al?


On 10/15/19 9:37 AM, Ritesh Harjani wrote:
> ping!!
> 
> On 9/27/19 10:12 AM, Ritesh Harjani wrote:
>> d_is_negative can race with d_instantiate_new()
>> -> __d_set_inode_and_type().
>> For e.g. in use cases where Thread-1 is creating
>> symlink (doing d_instantiate_new()) & Thread-2 is doing
>> cat of that symlink while doing lookup_fast (via REF-walk-
>> one such case is, when ->permission returns -ECHILD).
>>
>> During this race if __d_set_and_inode_type() does out-of-order
>> execution and set the dentry->d_flags before setting
>> dentry->inode, then it can result into following kernel panic.
>>
>> This change fixes the issue by directly checking for inode.
>>
>> E.g. kernel panic, since inode was NULL.
>> trailing_symlink() -> may_follow_link() -> inode->i_uid.
>> Issue signature:-
>>    [NIP  : trailing_symlink+80]
>>    [LR   : trailing_symlink+1092]
>>    #4 [c00000198069bb70] trailing_symlink at c0000000004bae60  
>> (unreliable)
>>    #5 [c00000198069bc00] path_openat at c0000000004bdd14
>>    #6 [c00000198069bc90] do_filp_open at c0000000004c0274
>>    #7 [c00000198069bdb0] do_sys_open at c00000000049b248
>>    #8 [c00000198069be30] system_call at c00000000000b388
>>
>> Sequence of events:-
>> Thread-2(Comm: ln)            Thread-1(Comm: cat)
>>
>>                     dentry = __d_lookup() //nonRCU
>>
>> __d_set_and_inode_type() (Out-of-order execution)
>>      flags = READ_ONCE(dentry->d_flags);
>>      flags &= ~(DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU);
>>      flags |= type_flags;
>>      WRITE_ONCE(dentry->d_flags, flags);
>>
>>                     if (unlikely(d_is_negative()) // fails
>>                            {}
>>                     // since d_flags is already updated in
>>                     // Thread-2 in parallel but inode
>>                     // not yet set.
>>                     // d_is_negative returns false
>>
>>                     *inode = d_backing_inode(path->dentry);
>>                     // means inode is still NULL
>>
>>      dentry->d_inode = inode;
>>
>>                     trailing_symlink()
>>                         may_follow_link()
>>                             inode = nd->link_inode;
>>                             // nd->link_inode = NULL
>>                             //Then it crashes while
>>                             //doing inode->i_uid
>>
>> Reported-by: Guang Yuan Wu <wugyuan@cn.ibm.com>
>> Tested-by: Guang Yuan Wu <wugyuan@cn.ibm.com>
>> Acked-by: Jeff Layton <jlayton@kernel.org>
>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
>> ---
>>   fs/namei.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 671c3c1a3425..7c5337cddebd 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -1617,7 +1617,21 @@ static int lookup_fast(struct nameidata *nd,
>>           dput(dentry);
>>           return status;
>>       }
>> -    if (unlikely(d_is_negative(dentry))) {
>> +
>> +    /*
>> +     * Caution: d_is_negative() can race with
>> +     * __d_set_inode_and_type().
>> +     * For e.g. in use cases where Thread-1 is creating
>> +     * symlink (doing d_instantiate_new()) & Thread-2 is doing
>> +     * cat of that symlink and falling here (via Ref-walk) while
>> +     * doing lookup_fast (one such case is when ->permission
>> +     * returns -ECHILD).
>> +     * Now if __d_set_inode_and_type() does out-of-order execution
>> +     * i.e. it first sets the dentry->d_flags & then dentry->inode
>> +     * then it can result into inode being NULL, causing panic later.
>> +     * Hence directly check if inode is NULL here.
>> +     */
>> +    if (unlikely(d_really_is_negative(dentry))) {
>>           dput(dentry);
>>           return -ENOENT;
>>       }
>>
>