Re: [PATCH v2] btrfs: Fix slab-use-after-free Read in add_ra_bio_pages

[Qu Wenruo <quwenruo.btrfs@xxxxxxx>]

在 2024/7/11 13:59, Pei Li 写道:
> We are accessing the start and len field in em after it is free'd.
>
> This patch moves the line accessing the free'd values in em before
> they were free'd so we won't access free'd memory.
>
> Reported-by: syzbot+853d80cba98ce1157ae6@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=853d80cba98ce1157ae6
> Signed-off-by: Pei Li <peili.dev@xxxxxxxxx>
> ---
> 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 moves the line accessing the free'd values in em before
> they were free'd so we won't access free'd memory.
>
> Fixes: 6a4049102055 ("btrfs: subpage: make add_ra_bio_pages() compatible")

This fixes tag, along with the syzbot report, should be in the main
commit message, not after the "---" line, which would be discarded when
applying.

> ---
> Changes in v2:
> - Adapt Qu's suggestion to move the read-after-free line before freeing
> - Cc stable kernel

It's not just Ccing to the stable list, but with a version tag.

For all the proper tags usage, you can check this commit, it has all the
correct tags.

b2a616676839 ("btrfs: fix rw device counting in __btrfs_free_extra_devids")

Otherwise the code looks good to me.

Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>

Thanks,
Qu

> - Link to v1: https://lore.kernel.org/r/20240710-bug11-v1-1-aa02297fbbc9@xxxxxxxxx
> ---
>   fs/btrfs/compression.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 6441e47d8a5e..f271df10ef1c 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -514,6 +514,8 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>   			put_page(page);
>   			break;
>   		}
> +		add_size = min(em->start + em->len, page_end + 1) - cur;
> +
>   		free_extent_map(em);
>
>   		if (page->index == end_index) {
> @@ -526,7 +528,6 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>   			}
>   		}
>
> -		add_size = min(em->start + em->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,