Re: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem

[Gu Zheng]

On 07/25/2014 11:22 AM, Chao Yu wrote:

> Hi,
> 
> To Andrey:
> Thanks for your test on this patch!
> 
> To Gu:
> If you do not object, let me make and resend a patch base on the one which
> skip invalidating pages.

Please go ahead.:)

Thanks,
Gu

> 
> Regards,
> Yu
> 
>> -----Original Message-----
>> From: Andrey Tsyvarev [mailto:tsyvarev@xxxxxxxxx]
>> Sent: Thursday, July 24, 2014 6:15 PM
>> To: Gu Zheng; Chao Yu
>> Cc: 'Jaegeuk Kim'; 'linux-kernel'; 'Alexey Khoroshilov';
>> linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem
>>
>> Hi,
>>
>> With patch skipping invalidating pages for node_inode and meta_inode
>> use-after-free error disappears too.
>>
>> 23.07.2014 7:39, Gu Zheng ÐÐÑÐÑ:
>>> Hi,
>>> On 07/23/2014 10:12 AM, Chao Yu wrote:
>>>
>>>> Hi Andrey Gu,
>>>>
>>>>> -----Original Message-----
>>>>> From: Andrey Tsyvarev [mailto:tsyvarev@xxxxxxxxx]
>>>>> Sent: Tuesday, July 22, 2014 6:04 PM
>>>>> To: Gu Zheng
>>>>> Cc: Jaegeuk Kim; linux-kernel; Alexey Khoroshilov; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>> Subject: Re: [f2fs-dev] f2fs: Possible use-after-free when umount filesystem
>>>>>
>>>>> Hi Gu,
>>>>>
>>>>>>> Investigation shows, that f2fs_evict_inode, when called for 'meta_inode', uses
>>>>> invalidate_mapping_pages() for 'node_inode'.
>>>>>>> But 'node_inode' is deleted before 'meta_inode' in f2fs_put_super via iput().
>>>>>>>
>>>>>>> It seems that in common usage scenario this use-after-free is benign, because 'node_inode'
>>>>> remains partially valid data even after kmem_cache_free().
>>>>>>> But things may change if, while 'meta_inode' is evicted in one f2fs filesystem, another
>> (mounted)
>>>>> f2fs filesystem requests inode from cache, and formely
>>>>>>> 'node_inode' of the first filesystem is returned.
>>>>>> The analysis seems reasonable. Have you tried to swap the reclaim order of node_inde
>>>>>> and meta_inode?
>>>>>>
>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>> index 870fe19..e114418 100644
>>>>>> --- a/fs/f2fs/super.c
>>>>>> +++ b/fs/f2fs/super.c
>>>>>> @@ -430,8 +430,8 @@ static void f2fs_put_super(struct super_block *sb)
>>>>>>           if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
>>>>>>                   write_checkpoint(sbi, true);
>>>>>>
>>>>>> -       iput(sbi->node_inode);
>>>>>>           iput(sbi->meta_inode);
>>>>>> +       iput(sbi->node_inode);
>>>>>>
>>>>>>           /* destroy f2fs internal modules */
>>>>>>           destroy_node_manager(sbi);
>>>>>>
>>>>>> Thanks,
>>>>>> Gu
>>>>> With reclaim order of node_inode and meta_inode swapped, use-after-free
>>>>> error disappears.
>>>>>
>>>>> But shouldn't initialization order of these inodes be swapped too?
>>>>> As meta_inode uses node_inode, it seems logical that it should be
>>>>> initialized after it.
>>> The initialization order dose not affect anything, so swapping the order dose not
>>> make more sense here.
>>>
>>>> IMO, it's not easy to exchange order of initialization between meta_inode and
>>>> node_inode, because we should use meta_inode in get_valid_checkpoint for valid
>>>> cp first for usual verification, then init node_inode.
>>> Yeah, but I think just moving node_inode's initialization to the front of meta_inode
>>> dose not break anything.
>>>
>>>> As I checked, nids for both meta_inode and node_inode are reservation, so it's not
>>>> necessary for us to invalidate pages which will never alloced.
>>>>
>>>> How about skipping it as following?
>>> It seems the right way to fix this issue.
>>>
>>> To Andrey:
>>> Could you please try this one?
>>>
>>> Thanks,
>>> Gu
>>>
>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>> index 2cf6962..cafba3c 100644
>>>> --- a/fs/f2fs/inode.c
>>>> +++ b/fs/f2fs/inode.c
>>>> @@ -273,7 +273,7 @@ void f2fs_evict_inode(struct inode *inode)
>>>>
>>>>   	if (inode->i_ino == F2FS_NODE_INO(sbi) ||
>>>>   			inode->i_ino == F2FS_META_INO(sbi))
>>>> -		goto no_delete;
>>>> +		goto out_clear;
>>>>
>>>>   	f2fs_bug_on(get_dirty_dents(inode));
>>>>   	remove_dirty_dir_inode(inode);
>>>> @@ -295,6 +295,7 @@ void f2fs_evict_inode(struct inode *inode)
>>>>
>>>>   	sb_end_intwrite(inode->i_sb);
>>>>   no_delete:
>>>> -	clear_inode(inode);
>>>>   	invalidate_mapping_pages(NODE_MAPPING(sbi), inode->i_ino, inode->i_ino);
>>>> +out_clear:
>>>> +	clear_inode(inode);
>>>>   }
>>>>
>>>>> --
>>>>> Best regards,
>>>>>
>>>>> Andrey Tsyvarev
>>>>> Linux Verification Center, ISPRAS
>>>>> web:http://linuxtesting.org
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> Want fast and easy access to all the code in your enterprise? Index and
>>>>> search up to 200,000 lines of code with a free copy of Black Duck
>>>>> Code Sight - the same software that powers the world's largest code
>>>>> search on Ohloh, the Black Duck Open Hub! Try it now.
>>>>> http://p.sf.net/sfu/bds
>>>>> _______________________________________________
>>>>> Linux-f2fs-devel mailing list
>>>>> Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>> .
>>>>
>>>
>>>
>>
>> --
>> Best regards,
>>
>> Andrey Tsyvarev
>> Linux Verification Center, ISPRAS
>> web:http://linuxtesting.org
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/