linux-ext4 - Re: [RFCv2 2/5] ext4: Remove PAGE_SIZE assumption of folio from mpage_submit

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

Message-ID: <ZIXXGhvfJEaKDIXC@casper.infradead.org>
Date:   Sun, 11 Jun 2023 15:15:54 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Theodore Ts'o <tytso@....edu>
Cc:     "Ritesh Harjani (IBM)" <ritesh.list@...il.com>,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Ojaswin Mujoo <ojaswin@...ux.ibm.com>,
        Disha Goel <disgoel@...ux.ibm.com>
Subject: Re: [RFCv2 2/5] ext4: Remove PAGE_SIZE assumption of folio from
 mpage_submit_folio

On Sun, Jun 11, 2023 at 01:58:31AM -0400, Theodore Ts'o wrote:
> On Mon, May 15, 2023 at 04:10:41PM +0530, Ritesh Harjani (IBM) wrote:
> > mpage_submit_folio() was converted to take folio. Even though
> > folio_size() in ext4 as of now is PAGE_SIZE, but it's better to
> > remove that assumption which I am assuming is a missed left over from
> > patch[1].
> > 
> > [1]: https://lore.kernel.org/linux-ext4/20230324180129.1220691-7-willy@infradead.org/
> > 
> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
> 
> I didn't notice this right away, because the failure is not 100%
> reliable, but this commit will sometimes cause "kvm-xfstests -c
> ext4/encrypt generic/068" to crash.  Reverting the patch fixes the
> problem, so I plan to drop this patch from my tree.

Hrm.  Well, let's think about how this can go wrong:

@@ -1885,7 +1885,7 @@ static int mpage_submit_folio(struct mpage_da_data *mpd,
+struct folio *folio)
        len = folio_size(folio);
        if (folio_pos(folio) + len > size &&
            !ext4_verity_in_progress(mpd->inode))
-               len = size & ~PAGE_MASK;
+        	len = size - folio_pos(folio);
        err = ext4_bio_write_folio(&mpd->io_submit, folio, len);
        if (!err)
                mpd->wbc->nr_to_write--;

Just off-camera is:

        size = i_size_read(mpd->inode);

Now, nothing is preventing i_size to be truncated to far below this
folio, right?  So if that happened before this patch, we'd write some
randomly sized fragment of the page.  Now we'll get a negative result
... which is assigned to size_t, so is exabytes in size.

So do we care if we write a random fragment of a page after a truncate?
If so, we should add:

	if (folio_pos(folio) >= size)
		return 0; /* Do we need to account nr_to_write? */

If we simply don't care that we're doing a spurious write, then we can
do something like:

-		len = size & ~PAGE_MASK;
+		len = size & (len - 1);