[PATCH] btrfs: fix a out-of-boundary access for copy_compressed_data_to_page()

Subsystems: btrfs file system, filesystems (vfs and infrastructure), the rest

STALE1795d

5 messages, 5 authors, 2021-11-12 · open the first message on its own page

[PATCH] btrfs: fix a out-of-boundary access for copy_compressed_data_to_page()

From: Qu Wenruo <hidden>
Date: 2021-11-12 02:23:15

[BUG]
The following script can cause btrfs to crash:

 mount -o compress-force=lzo $DEV /mnt
 dd if=/dev/urandom of=/mnt/foo bs=4k count=1
 sync

The calltrace looks like this:

 general protection fault, probably for non-canonical address 0xe04b37fccce3b000: 0000 [#1] PREEMPT SMP NOPTI
 CPU: 5 PID: 164 Comm: kworker/u20:3 Not tainted 5.15.0-rc7-custom+ #4
 Workqueue: btrfs-delalloc btrfs_work_helper [btrfs]
 RIP: 0010:__memcpy+0x12/0x20
 Call Trace:
  lzo_compress_pages+0x236/0x540 [btrfs]
  btrfs_compress_pages+0xaa/0xf0 [btrfs]
  compress_file_range+0x431/0x8e0 [btrfs]
  async_cow_start+0x12/0x30 [btrfs]
  btrfs_work_helper+0xf6/0x3e0 [btrfs]
  process_one_work+0x294/0x5d0
  worker_thread+0x55/0x3c0
  kthread+0x140/0x170
  ret_from_fork+0x22/0x30
 ---[ end trace 63c3c0f131e61982 ]---

[CAUSE]
In lzo_compress_pages(), parameter @out_pages is not only an output
parameter (for the compressed pages), but also an input parameter, for
the maximum amount of pages we can utilize.

In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
compatible"), the refactor doesn't take @out_pages as an input, thus
completely ignoring the limit.

And for compress-force case, we could hit incompressible data that
compressed size would go beyond the page limit, and cause above crash.

[FIX]
Save @out_pages as @max_nr_page, and pass it to lzo_compress_pages(),
and check if we're beyond the limit before accessing the pages.

Reported-by: Omar Sandoval <redacted>
Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible")
Signed-off-by: Qu Wenruo <redacted>
---
 fs/btrfs/lzo.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 00cffc183ec0..f410ceabcdbd 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -125,6 +125,7 @@ static inline size_t read_compress_length(const char *buf)
 static int copy_compressed_data_to_page(char *compressed_data,
 					size_t compressed_size,
 					struct page **out_pages,
+					unsigned long max_nr_page,
 					u32 *cur_out,
 					const u32 sectorsize)
 {
@@ -132,6 +133,9 @@ static int copy_compressed_data_to_page(char *compressed_data,
 	u32 orig_out;
 	struct page *cur_page;
 
+	if ((*cur_out / PAGE_SIZE) >= max_nr_page)
+		return -E2BIG;
+
 	/*
 	 * We never allow a segment header crossing sector boundary, previous
 	 * run should ensure we have enough space left inside the sector.
@@ -158,6 +162,9 @@ static int copy_compressed_data_to_page(char *compressed_data,
 		u32 copy_len = min_t(u32, sectorsize - *cur_out % sectorsize,
 				     orig_out + compressed_size - *cur_out);
 
+		if ((*cur_out / PAGE_SIZE) >= max_nr_page)
+			return -E2BIG;
+
 		cur_page = out_pages[*cur_out / PAGE_SIZE];
 		/* Allocate a new page */
 		if (!cur_page) {
@@ -195,6 +202,7 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 	const u32 sectorsize = btrfs_sb(mapping->host->i_sb)->sectorsize;
 	struct page *page_in = NULL;
+	const unsigned long max_nr_page = *out_pages;
 	int ret = 0;
 	/* Points to the file offset of input data */
 	u64 cur_in = start;
@@ -202,6 +210,7 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
 	u32 cur_out = 0;
 	u32 len = *total_out;
 
+	ASSERT(max_nr_page > 0);
 	*out_pages = 0;
 	*total_out = 0;
 	*total_in = 0;
@@ -237,7 +246,8 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
 		}
 
 		ret = copy_compressed_data_to_page(workspace->cbuf, out_len,
-						   pages, &cur_out, sectorsize);
+						   pages, max_nr_page,
+						   &cur_out, sectorsize);
 		if (ret < 0)
 			goto out;
 
-- 
2.33.1

Re: [PATCH] btrfs: fix a out-of-boundary access for copy_compressed_data_to_page()

From: Josef Bacik <josef@toxicpanda.com>
Date: 2021-11-12 04:17:07

On Fri, Nov 12, 2021 at 10:22:53AM +0800, Qu Wenruo wrote:
quoted hunk
[BUG]
The following script can cause btrfs to crash:

 mount -o compress-force=lzo $DEV /mnt
 dd if=/dev/urandom of=/mnt/foo bs=4k count=1
 sync

The calltrace looks like this:

 general protection fault, probably for non-canonical address 0xe04b37fccce3b000: 0000 [#1] PREEMPT SMP NOPTI
 CPU: 5 PID: 164 Comm: kworker/u20:3 Not tainted 5.15.0-rc7-custom+ #4
 Workqueue: btrfs-delalloc btrfs_work_helper [btrfs]
 RIP: 0010:__memcpy+0x12/0x20
 Call Trace:
  lzo_compress_pages+0x236/0x540 [btrfs]
  btrfs_compress_pages+0xaa/0xf0 [btrfs]
  compress_file_range+0x431/0x8e0 [btrfs]
  async_cow_start+0x12/0x30 [btrfs]
  btrfs_work_helper+0xf6/0x3e0 [btrfs]
  process_one_work+0x294/0x5d0
  worker_thread+0x55/0x3c0
  kthread+0x140/0x170
  ret_from_fork+0x22/0x30
 ---[ end trace 63c3c0f131e61982 ]---

[CAUSE]
In lzo_compress_pages(), parameter @out_pages is not only an output
parameter (for the compressed pages), but also an input parameter, for
the maximum amount of pages we can utilize.

In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
compatible"), the refactor doesn't take @out_pages as an input, thus
completely ignoring the limit.

And for compress-force case, we could hit incompressible data that
compressed size would go beyond the page limit, and cause above crash.

[FIX]
Save @out_pages as @max_nr_page, and pass it to lzo_compress_pages(),
and check if we're beyond the limit before accessing the pages.

Reported-by: Omar Sandoval <redacted>
Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible")
Signed-off-by: Qu Wenruo <redacted>
---
 fs/btrfs/lzo.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 00cffc183ec0..f410ceabcdbd 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -125,6 +125,7 @@ static inline size_t read_compress_length(const char *buf)
 static int copy_compressed_data_to_page(char *compressed_data,
 					size_t compressed_size,
 					struct page **out_pages,
+					unsigned long max_nr_page,
If you want to do const down below you should use const here probably?  Thanks,

Josef

Re: [PATCH] btrfs: fix a out-of-boundary access for copy_compressed_data_to_page()

From: Omar Sandoval <osandov@osandov.com>
Date: 2021-11-12 04:26:55

On Fri, Nov 12, 2021 at 10:22:53AM +0800, Qu Wenruo wrote:
[BUG]
The following script can cause btrfs to crash:

 mount -o compress-force=lzo $DEV /mnt
 dd if=/dev/urandom of=/mnt/foo bs=4k count=1
 sync

The calltrace looks like this:

 general protection fault, probably for non-canonical address 0xe04b37fccce3b000: 0000 [#1] PREEMPT SMP NOPTI
 CPU: 5 PID: 164 Comm: kworker/u20:3 Not tainted 5.15.0-rc7-custom+ #4
 Workqueue: btrfs-delalloc btrfs_work_helper [btrfs]
 RIP: 0010:__memcpy+0x12/0x20
 Call Trace:
  lzo_compress_pages+0x236/0x540 [btrfs]
  btrfs_compress_pages+0xaa/0xf0 [btrfs]
  compress_file_range+0x431/0x8e0 [btrfs]
  async_cow_start+0x12/0x30 [btrfs]
  btrfs_work_helper+0xf6/0x3e0 [btrfs]
  process_one_work+0x294/0x5d0
  worker_thread+0x55/0x3c0
  kthread+0x140/0x170
  ret_from_fork+0x22/0x30
 ---[ end trace 63c3c0f131e61982 ]---

[CAUSE]
In lzo_compress_pages(), parameter @out_pages is not only an output
parameter (for the compressed pages), but also an input parameter, for
the maximum amount of pages we can utilize.

In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
compatible"), the refactor doesn't take @out_pages as an input, thus
completely ignoring the limit.

And for compress-force case, we could hit incompressible data that
compressed size would go beyond the page limit, and cause above crash.

[FIX]
Save @out_pages as @max_nr_page, and pass it to lzo_compress_pages(),
and check if we're beyond the limit before accessing the pages.

Reported-by: Omar Sandoval <redacted>
Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible")
Signed-off-by: Qu Wenruo <redacted>
---
 fs/btrfs/lzo.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
This fixed the issue for me, and it looks correct.

Reviewed-by: Omar Sandoval <redacted>

Re: [PATCH] btrfs: fix a out-of-boundary access for copy_compressed_data_to_page()

From: Qu Wenruo <hidden>
Date: 2021-11-12 04:41:50


On 2021/11/12 12:17, Josef Bacik wrote:
On Fri, Nov 12, 2021 at 10:22:53AM +0800, Qu Wenruo wrote:
quoted
[BUG]
The following script can cause btrfs to crash:

  mount -o compress-force=lzo $DEV /mnt
  dd if=/dev/urandom of=/mnt/foo bs=4k count=1
  sync

The calltrace looks like this:

  general protection fault, probably for non-canonical address 0xe04b37fccce3b000: 0000 [#1] PREEMPT SMP NOPTI
  CPU: 5 PID: 164 Comm: kworker/u20:3 Not tainted 5.15.0-rc7-custom+ #4
  Workqueue: btrfs-delalloc btrfs_work_helper [btrfs]
  RIP: 0010:__memcpy+0x12/0x20
  Call Trace:
   lzo_compress_pages+0x236/0x540 [btrfs]
   btrfs_compress_pages+0xaa/0xf0 [btrfs]
   compress_file_range+0x431/0x8e0 [btrfs]
   async_cow_start+0x12/0x30 [btrfs]
   btrfs_work_helper+0xf6/0x3e0 [btrfs]
   process_one_work+0x294/0x5d0
   worker_thread+0x55/0x3c0
   kthread+0x140/0x170
   ret_from_fork+0x22/0x30
  ---[ end trace 63c3c0f131e61982 ]---

[CAUSE]
In lzo_compress_pages(), parameter @out_pages is not only an output
parameter (for the compressed pages), but also an input parameter, for
the maximum amount of pages we can utilize.

In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
compatible"), the refactor doesn't take @out_pages as an input, thus
completely ignoring the limit.

And for compress-force case, we could hit incompressible data that
compressed size would go beyond the page limit, and cause above crash.

[FIX]
Save @out_pages as @max_nr_page, and pass it to lzo_compress_pages(),
and check if we're beyond the limit before accessing the pages.

Reported-by: Omar Sandoval <redacted>
Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible")
Signed-off-by: Qu Wenruo <redacted>
---
  fs/btrfs/lzo.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 00cffc183ec0..f410ceabcdbd 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -125,6 +125,7 @@ static inline size_t read_compress_length(const char *buf)
  static int copy_compressed_data_to_page(char *compressed_data,
  					size_t compressed_size,
  					struct page **out_pages,
+					unsigned long max_nr_page,
If you want to do const down below you should use const here probably?  Thanks,
Right, max_nr_page should also be const.

Thanks for catching this,
Qu
Josef

Re: [PATCH] btrfs: fix a out-of-boundary access for copy_compressed_data_to_page()

From: David Sterba <hidden>
Date: 2021-11-12 14:35:08

On Fri, Nov 12, 2021 at 12:41:37PM +0800, Qu Wenruo wrote:

On 2021/11/12 12:17, Josef Bacik wrote:
quoted
On Fri, Nov 12, 2021 at 10:22:53AM +0800, Qu Wenruo wrote:
quoted
[BUG]
The following script can cause btrfs to crash:

  mount -o compress-force=lzo $DEV /mnt
  dd if=/dev/urandom of=/mnt/foo bs=4k count=1
  sync

The calltrace looks like this:

  general protection fault, probably for non-canonical address 0xe04b37fccce3b000: 0000 [#1] PREEMPT SMP NOPTI
  CPU: 5 PID: 164 Comm: kworker/u20:3 Not tainted 5.15.0-rc7-custom+ #4
  Workqueue: btrfs-delalloc btrfs_work_helper [btrfs]
  RIP: 0010:__memcpy+0x12/0x20
  Call Trace:
   lzo_compress_pages+0x236/0x540 [btrfs]
   btrfs_compress_pages+0xaa/0xf0 [btrfs]
   compress_file_range+0x431/0x8e0 [btrfs]
   async_cow_start+0x12/0x30 [btrfs]
   btrfs_work_helper+0xf6/0x3e0 [btrfs]
   process_one_work+0x294/0x5d0
   worker_thread+0x55/0x3c0
   kthread+0x140/0x170
   ret_from_fork+0x22/0x30
  ---[ end trace 63c3c0f131e61982 ]---

[CAUSE]
In lzo_compress_pages(), parameter @out_pages is not only an output
parameter (for the compressed pages), but also an input parameter, for
the maximum amount of pages we can utilize.

In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
compatible"), the refactor doesn't take @out_pages as an input, thus
completely ignoring the limit.

And for compress-force case, we could hit incompressible data that
compressed size would go beyond the page limit, and cause above crash.

[FIX]
Save @out_pages as @max_nr_page, and pass it to lzo_compress_pages(),
and check if we're beyond the limit before accessing the pages.

Reported-by: Omar Sandoval <redacted>
Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible")
Signed-off-by: Qu Wenruo <redacted>
---
  fs/btrfs/lzo.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 00cffc183ec0..f410ceabcdbd 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -125,6 +125,7 @@ static inline size_t read_compress_length(const char *buf)
  static int copy_compressed_data_to_page(char *compressed_data,
  					size_t compressed_size,
  					struct page **out_pages,
+					unsigned long max_nr_page,
If you want to do const down below you should use const here probably?  Thanks,
Right, max_nr_page should also be const.
const for non-pointer parameters does not make much sense, it only
prevents reuse of the variable inside the function.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help