On 2021/3/30 23:02, Jan Kara wrote: > On Mon 29-03-21 17:20:35, Zhang Yi wrote: >> On 2021/3/23 1:25, Jan Kara wrote: >>> Hi! >>> >>> On Mon 22-03-21 23:24:23, Zhang Yi wrote: >>>> We find a use after free problem when CONFIG_QUOTA is enabled, the detail of >>>> this problem is below. >>>> >>>> mount_bdev() >>>> ext4_fill_super() >>>> sb->s_root = d_make_root(root); >>>> ext4_orphan_cleanup() >>>> sb->s_flags |= SB_ACTIVE; <--- 1. mark sb active >>>> ext4_orphan_get() >>>> ext4_truncate() >>>> ext4_block_truncate_page() >>>> mark_buffer_dirty <--- 2. dirty inode >>>> iput() >>>> iput_final <--- 3. put into lru list >>>> ext4_mark_recovery_complete <--- 4. failed and return error >>>> sb->s_root = NULL; >>>> deactivate_locked_super() >>>> kill_block_super() >>>> generic_shutdown_super() >>>> <--- 5. did not evict_inodes >>>> put_super() >>>> __put_super() >>>> <--- 6. put super block >>>> >>>> Because of the truncated inodes was dirty and will write them back later, it >>>> will trigger use after free problem. Now the question is why we need to set >>>> SB_ACTIVE bit when enable CONFIG_QUOTA below? >>>> >>>> #ifdef CONFIG_QUOTA >>>> /* Needed for iput() to work correctly and not trash data */ >>>> sb->s_flags |= SB_ACTIVE; >>>> >>>> This code was merged long long ago in v2.6.6, IIUC, it may not affect >>>> the quota statistics it we evict inode directly in the last iput. >>>> In order to slove this UAF problem, I'm not sure is there any side effect >>>> if we just remove this code, or remove SB_ACTIVE and call evict_inodes() >>>> in the error path of ext4_fill_super(). >>>> >>>> Could you give some suggestions? >>> >>> That's a very good question. I do remember that I've added this code back >>> then because otherwise orphan cleanup was loosing updates to quota files. >>> But you're right that now I don't see how that could be happening and it >>> would be nice if we could get rid of this hack (and even better if it also >>> fixes the problem you've found). I guess I'll just try and test this change >>> with various quota configurations to see whether something still breaks or >>> not. Thanks report! >>> >> >> Thanks for taking time to look at this, is this change OK under your various >> quota test cases? > > Yes, I did tests both with journalled quotas and with ext4 quota feature > and the quota accounting was correct after orphan recovery. So just > removing the SB_ACTIVE setting is fine AFAICT. Will you send a patch or > should I do it? > Thanks for testing this change, I will send a patch. Yi.
