linux-ext4 - Re: set_page_dirty vs truncate

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-next>] [day] [month] [year] [list]

Message-ID: <20201218220316.GO15600@casper.infradead.org>
Date:   Fri, 18 Dec 2020 22:03:16 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     linux-fsdevel@...r.kernel.org
Cc:     linux-ext4@...r.kernel.org, Jan Kara <jack@...e.com>,
        Theodore Ts'o <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>
Subject: Re: set_page_dirty vs truncate

On Fri, Dec 18, 2020 at 04:05:31PM +0000, Matthew Wilcox wrote:
> A number of implementations of ->set_page_dirty check whether the page
> has been truncated (ie page->mapping has become NULL since entering
> set_page_dirty()).  Several other implementations assume that they can do
> page->mapping->host to get to the inode.  So either some implementations
> are doing unnecessary checks or others are vulnerable to a NULL pointer
> dereference if truncate() races with set_page_dirty().
> 
> I'm touching ->set_page_dirty() anyway as part of the page folio
> conversion.  I'm thinking about passing in the mapping so there's no
> need to look at page->mapping.
> 
> The comments on set_page_dirty() and set_page_dirty_lock() suggests
> there's no consistency in whether truncation is blocked or not; we're
> only guaranteed that the inode itself won't go away.  But maybe the
> comments are stale.

The comments are, I believe, not stale.  Here's some syzbot
reports which indicate that ext4 is seeing races between set_page_dirty()
and truncate():

 https://groups.google.com/g/syzkaller-lts-bugs/c/s9fHu162zhQ/m/Phnf6ucaAwAJ

The reproducer includes calls to ftruncate(), so that would suggest
that's what's going on.

I would suggest just deleting this line:

        WARN_ON_ONCE(!page_has_buffers(page));

I'm not sure what value the other WARN_ON_ONCE adds.  Maybe just replace
ext4_set_page_dirty with __set_page_dirty_buffers in the aops?  I'd defer
to an ext4 expert on this ...