| Message ID | 20240820232622.19271-1-phillip@squashfs.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, 21 Aug 2024 00:26:22 +0100, Phillip Lougher wrote: > In the recent work to remove page->index, a sanity check > that ensured all the readhead pages were covered by the > Squashfs data block was removed [1]. > > To avoid any regression, this commit adds the sanity check > back in an equivalent way. Namely the page actor will now > return error if any pages are unused after completion. > > [...] Applied to the vfs.folio branch of the vfs/vfs.git tree. Patches in the vfs.folio branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.folio [5/5] Squashfs: Ensure all readahead pages have been used https://git.kernel.org/vfs/vfs/c/5d85f9c952d8
On 8/21/2024 1:12 AM, Christian Brauner wrote: > On Wed, 21 Aug 2024 00:26:22 +0100, Phillip Lougher wrote: >> In the recent work to remove page->index, a sanity check >> that ensured all the readhead pages were covered by the >> Squashfs data block was removed [1]. >> >> To avoid any regression, this commit adds the sanity check >> back in an equivalent way. Namely the page actor will now >> return error if any pages are unused after completion. >> >> [...] > > Applied to the vfs.folio branch of the vfs/vfs.git tree. > Patches in the vfs.folio branch should appear in linux-next soon. > > Please report any outstanding bugs that were missed during review in a > new review to the original patch series allowing us to drop it. > > It's encouraged to provide Acked-bys and Reviewed-bys even though the > patch has now been applied. If possible patch trailers will be updated. > > Note that commit hashes shown below are subject to change due to rebase, > trailer updates or similar. If in doubt, please check the listed branch. > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git > branch: vfs.folio > > [5/5] Squashfs: Ensure all readahead pages have been used > https://git.kernel.org/vfs/vfs/c/5d85f9c952d8 > When I was testing the linux-next branch I got "BUG: KASAN: slab-use-after-free in squashfs_readahead+0x19f1/0x1e50" It seems this is due to the access of `actor` just after freeing it. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/fs/squashfs/page_actor.h#n41 -Fan
diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c index 5a3745e52025..21aaa96856c1 100644 --- a/fs/squashfs/file.c +++ b/fs/squashfs/file.c @@ -535,7 +535,7 @@ static int squashfs_readahead_fragment(struct page **page, last_page = squashfs_page_actor_free(actor); - if (copied == expected) { + if (copied == expected && !IS_ERR(last_page)) { /* Last page (if present) may have trailing bytes not filled */ bytes = copied % PAGE_SIZE; if (bytes && last_page) @@ -625,7 +625,7 @@ static void squashfs_readahead(struct readahead_control *ractl) last_page = squashfs_page_actor_free(actor); - if (res == expected) { + if (res == expected && !IS_ERR(last_page)) { int bytes; /* Last page (if present) may have trailing bytes not filled */ diff --git a/fs/squashfs/file_direct.c b/fs/squashfs/file_direct.c index 646d4d421f99..22251743fadf 100644 --- a/fs/squashfs/file_direct.c +++ b/fs/squashfs/file_direct.c @@ -80,7 +80,7 @@ int squashfs_readpage_block(struct page *target_page, u64 block, int bsize, if (res < 0) goto mark_errored; - if (res != expected) { + if (res != expected || IS_ERR(last_page)) { res = -EIO; goto mark_errored; } diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h index c6d837f0e9ca..f770a906c2c6 100644 --- a/fs/squashfs/page_actor.h +++ b/fs/squashfs/page_actor.h @@ -37,7 +37,11 @@ static inline struct page *squashfs_page_actor_free(struct squashfs_page_actor * kfree(actor->tmp_buffer); kfree(actor); - return last_page; + + if (actor->next_page == actor->pages) + return last_page; + else + return ERR_PTR(-EIO); } static inline void *squashfs_first_page(struct squashfs_page_actor *actor) {
In the recent work to remove page->index, a sanity check that ensured all the readhead pages were covered by the Squashfs data block was removed [1]. To avoid any regression, this commit adds the sanity check back in an equivalent way. Namely the page actor will now return error if any pages are unused after completion. [1] https://lore.kernel.org/all/20240818235847.170468-3-phillip@squashfs.org.uk/ Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk> --- fs/squashfs/file.c | 4 ++-- fs/squashfs/file_direct.c | 2 +- fs/squashfs/page_actor.h | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-)