linux-ext4 - Re: [PATCH] ext4: do not set SB_ACTIVE in ext4_orphan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YHCETyV70FNVO5u6@mit.edu>
Date:   Fri, 9 Apr 2021 12:43:59 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Jan Kara <jack@...e.cz>
Cc:     Zhang Yi <yi.zhang@...wei.com>, linux-ext4@...r.kernel.org,
        adilger.kernel@...ger.ca
Subject: Re: [PATCH] ext4: do not set SB_ACTIVE in ext4_orphan_cleanup()

On Wed, Mar 31, 2021 at 11:59:20AM +0200, Jan Kara wrote:
> On Wed 31-03-21 11:31:38, Zhang Yi wrote:
> > When CONFIG_QUOTA is enabled, if we failed to mount the filesystem due
> > to some error happens behind ext4_orphan_cleanup(), it will end up
> > triggering a after free issue of super_block. The problem is that
> > ext4_orphan_cleanup() will set SB_ACTIVE flag if CONFIG_QUOTA is
> > enabled, after we cleanup the truncated inodes, the last iput() will put
> > them into the lru list, and these inodes' pages may probably dirty and
> > will be write back by the writeback thread, so it could be raced by
> > freeing super_block in the error path of mount_bdev().
> > 
> > After check the setting of SB_ACTIVE flag in ext4_orphan_cleanup(), it
> > was used to ensure updating the quota file properly, but evict inode and
> > trash data immediately in the last iput does not affect the quotafile,
> > so setting the SB_ACTIVE flag seems not required[1]. Fix this issue by
> > just remove the SB_ACTIVE setting.
> 
> Thanks for the patch. Let me rephrase the changelog a little:
> 
> When CONFIG_QUOTA is enabled and if we later fail to finish mounting the
> filesystem due to some error after ext4_orphan_cleanup(), we may hit use
> after free issues. The problem is that ext4_orphan_cleanup() sets SB_ACTIVE
> flag and so inodes processed during the orphan cleanup are put to the
> superblock's LRU list instead of being immediately destroyed. However the
> path handling error recovery after failed ->fill_super() call does not
> destroy inodes attached to the superblock and so they are left active in
> memory while the superblock is freed.
> 
> Originally, SB_ACTIVE setting was added so that updated quota information
> is not destroyed when we drop quota inode references after orphan cleanup.
> However VFS does not purge dirty inode pages without SB_ACTIVE flag for many
> years already. So just remove the hack with setting SB_ACTIVE flag from
> ext4_orphan_cleanup().
> 
> Also feel free to add:
> 
> Reviewed-by: Jan Kara <jack@...e.cz>
> 
> 								Honza

Applied, thanks.

					- Ted