btrfs: Fix slab-use-after-free Read in add_ra_bio_pages

PATCH In review Pei Li 4 participants 4 messages Jul 11, 2024

Subsystems (2)

Stable branch Btrfs file system

lore

Share & tools

lore

Pei Li

author

2024-07-11 04:29 UTC #0 lore raw

PATCH v2 1/1 btrfs: Fix slab-use-after-free Read in add_ra_bio_pages

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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=853d80cba98ce1157ae6

Signed-off-by Pei Li <[email protected]>

---
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")

---
Changes in v2:
- Adapt Qu's suggestion to move the read-after-free line before freeing
- Cc stable kernel
- Link to v1: https://lore.kernel.org/r/[email protected]

---
 fs/btrfs/compression.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/btrfs/compression.c +2 -1

index 6441e47d8a5e..f271df10ef1c 100644
@@ -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,

Signature · 1 line

Pei Li <[email protected]>

kernel test robot

build bot

2024-07-11 04:32 UTC #1 lore raw

FYI: kernel test robot notices the stable kernel rule is not satisfied.

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1

Rule: add the tag "Cc: [email protected]" in the sign-off area to have the patch automatically included in the stable tree.
Subject: [PATCH v2] btrfs: Fix slab-use-after-free Read in add_ra_bio_pages

Link https://lore.kernel.org/stable/20240710-bug11-v2-1-e7bc61f32e5d%40gmail.com open here

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Qu Wenruo

Reviewed-by on v2 1/1

2024-07-11 05:29 UTC #2 lore raw

In reply to Pei Li

Quoted 12 lines · 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 [email protected]
Closes https://syzkaller.appspot.com/bug?extid=853d80cba98ce1157ae6
Signed-off-by Pei Li <[email protected]>
---
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

Quoted 7 lines

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.

Quoted 4 lines

---
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 <[email protected]>

Thanks,
Qu

Quoted 32 lines

- Link to v1: https://lore.kernel.org/r/[email protected]

---
  fs/btrfs/compression.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

fs/btrfs/compression.c +2 -2

index 6441e47d8a5e..f271df10ef1c 100644
@@ -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,

Filipe Manana

2024-07-11 06:39 UTC #3 lore raw

In reply to Pei Li

Quoted 13 lines · 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 [email protected]
Closes https://syzkaller.appspot.com/bug?extid=853d80cba98ce1157ae6
Signed-off-by Pei Li <[email protected]>
---
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

Quoted 7 lines

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 type of useful information should be in the changelog, not after the ---

And btw, this was already fixed last week and it's in for-next:

https://github.com/btrfs/linux/commit/aaa2c8b3f54e7b4f31616fd03bb302cc17cccf39
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m9a92a5d980230323ec351a24adf9a3738cfbbc40

Thanks.

Quoted 40 lines

---
Changes in v2:
- Adapt Qu's suggestion to move the read-after-free line before freeing
- Cc stable kernel
- Link to v1: https://lore.kernel.org/r/[email protected]

---
 fs/btrfs/compression.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/btrfs/compression.c +2 -2

index 6441e47d8a5e..f271df10ef1c 100644
@@ -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,
--
Pei Li <[email protected]>