[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7ba1b64f-7969-4c27-87ae-33c2c9c6b236@gmx.com>
Date: Thu, 11 Jul 2024 13:26:57 +0930
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Pei Li <peili.dev@...il.com>, Chris Mason <clm@...com>,
Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>
Cc: linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
skhan@...uxfoundation.org, syzkaller-bugs@...glegroups.com,
linux-kernel-mentees@...ts.linuxfoundation.org,
syzbot+853d80cba98ce1157ae6@...kaller.appspotmail.com
Subject: Re: [PATCH] btrfs: Fix slab-use-after-free Read in add_ra_bio_pages
在 2024/7/11 13:12, Pei Li 写道:
> We are accessing the start and len field in em after it is free'd.
>
> This patch stores the values that we are going to access from em before
> it was free'd so we won't access free'd memory.
>
> Reported-by: syzbot+853d80cba98ce1157ae6@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=853d80cba98ce1157ae6
> Signed-off-by: Pei Li <peili.dev@...il.com>
And missing the fixes tag:
Fixes: 6a4049102055 ("btrfs: subpage: make add_ra_bio_pages() compatible")
And it is better to Cc to stable kernel for v5.16 and newer kernels.
> ---
> Syzbot reported the following error:
> BUG: KASAN: slab-use-after-free in add_ra_bio_pages.constprop.0.isra.0+0xf03/0xfb0 fs/btrfs/compression.c:529
>
> This is because we are reading the values from em right after freeing it
> before through free_extent_map(em).
>
> This patch stores the values that we are going to access from em before
> it was free'd so we won't access free'd memory.
Indeed it's a use after free.
But the only usage is in this line:
add_size = min(em->start + em->len, page_end + 1) - cur;
So why not just move that line before "free_extent_map(em);"?
It should be much easier to read than the current change.
Thanks,
Qu
> ---
> fs/btrfs/compression.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 6441e47d8a5e..42b528aee63b 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -449,6 +449,7 @@ static noinline int add_ra_bio_pages(struct inode *inode,
> u64 page_end;
> u64 pg_index = cur >> PAGE_SHIFT;
> u32 add_size;
> + u64 start = 0, len = 0;
>
> if (pg_index > end_index)
> break;
> @@ -500,12 +501,17 @@ static noinline int add_ra_bio_pages(struct inode *inode,
> em = lookup_extent_mapping(em_tree, cur, page_end + 1 - cur);
> read_unlock(&em_tree->lock);
>
> + if (em) {
> + start = em->start;
> + len = em->len;
> + }
> +
> /*
> * At this point, we have a locked page in the page cache for
> * these bytes in the file. But, we have to make sure they map
> * to this compressed extent on disk.
> */
> - if (!em || cur < em->start ||
> + if (!em || cur < start ||
> (cur + fs_info->sectorsize > extent_map_end(em)) ||
> (em->block_start >> SECTOR_SHIFT) != orig_bio->bi_iter.bi_sector) {
> free_extent_map(em);
> @@ -526,7 +532,7 @@ static noinline int add_ra_bio_pages(struct inode *inode,
> }
> }
>
> - add_size = min(em->start + em->len, page_end + 1) - cur;
> + add_size = min(start + len, page_end + 1) - cur;
> ret = bio_add_page(orig_bio, page, add_size, offset_in_page(cur));
> if (ret != add_size) {
> unlock_extent(tree, cur, page_end, NULL);
>
> ---
> base-commit: 563a50672d8a86ec4b114a4a2f44d6e7ff855f5b
> change-id: 20240710-bug11-a8ac18afb724
>
> Best regards,
Powered by blists - more mailing lists