[PATCH] exfat: fix random stack corruption after get_block

[PATCH] exfat: fix random stack corruption after get_block

[Sungjong Seo]

When get_block is called with a buffer_head allocated on the stack, such
as do_mpage_readpage, stack corruption due to buffer_head UAF may occur in
the following race condition situation.

     <CPU 0>                      <CPU 1>
mpage_read_folio
  <<bh on stack>>
  do_mpage_readpage
    exfat_get_block
      bh_read
        __bh_read
	  get_bh(bh)
          submit_bh
          wait_on_buffer
                              ...
                              end_buffer_read_sync
                                __end_buffer_read_notouch
                                   unlock_buffer
          <<keep going>>
        ...
      ...
    ...
  ...
<<bh is not valid out of mpage_read_folio>>
   .
   .
another_function
  <<variable A on stack>>
                                   put_bh(bh)
                                     atomic_dec(bh->b_count)
  * stack corruption here *

This patch returns -EAGAIN if a folio does not have buffers when bh_read
needs to be called. By doing this, the caller can fallback to functions
like block_read_full_folio(), create a buffer_head in the folio, and then
call get_block again.

Let's do not call bh_read() with on-stack buffer_head.

Fixes: 11a347fb6cef ("exfat: change to get file size from DataLength")
Cc: stable@vger.kernel.org
Tested-by: Yeongjin Gil <youngjin.gil@samsung.com>
Signed-off-by: Sungjong Seo <sj1557.seo@samsung.com>
---
 fs/exfat/inode.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index 96952d4acb50..b8ea910586e4 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -344,7 +344,8 @@ static int exfat_get_block(struct inode *inode, sector_t iblock,
 			 * The block has been partially written,
 			 * zero the unwritten part and map the block.
 			 */
-			loff_t size, off, pos;
+			loff_t size, pos;
+			void *addr;
 
 			max_blocks = 1;
 
@@ -355,17 +356,43 @@ static int exfat_get_block(struct inode *inode, sector_t iblock,
 			if (!bh_result->b_folio)
 				goto done;
 
+			/*
+			 * No buffer_head is allocated.
+			 * (1) bmap: It's enough to fill bh_result without I/O.
+			 * (2) read: The unwritten part should be filled with 0
+			 *           If a folio does not have any buffers,
+			 *           let's returns -EAGAIN to fallback to
+			 *           per-bh IO like block_read_full_folio().
+			 */
+			if (!folio_buffers(bh_result->b_folio)) {
+				err = -EAGAIN;
+				goto done;
+			}
+
 			pos = EXFAT_BLK_TO_B(iblock, sb);
 			size = ei->valid_size - pos;
-			off = pos & (PAGE_SIZE - 1);
+			addr = folio_address(bh_result->b_folio) +
+			       offset_in_folio(bh_result->b_folio, pos);
+
+			BUG_ON(size > sb->s_blocksize);
+
+			/* Check if bh->b_data points to proper addr in folio */
+			if (bh_result->b_data != addr) {
+				exfat_fs_error_ratelimit(sb,
+					"b_data(%p) != folio_addr(%p)",
+					bh_result->b_data, addr);
+				err = -EINVAL;
+				goto done;
+			}
 
-			folio_set_bh(bh_result, bh_result->b_folio, off);
+			/* Read a block */
 			err = bh_read(bh_result, 0);
 			if (err < 0)
 				goto unlock_ret;
 
-			folio_zero_segment(bh_result->b_folio, off + size,
-					off + sb->s_blocksize);
+			/* Zero unwritten part of a block */
+			memset(bh_result->b_data + size, 0,
+			       bh_result->b_size - size);
 		} else {
 			/*
 			 * The range has not been written, clear the mapped flag
-- 
2.25.1

Re: [PATCH] exfat: fix random stack corruption after get_block

[Yuezhang.Mo]

+                       /*
+                        * No buffer_head is allocated.
+                        * (1) bmap: It's enough to fill bh_result without I/O.
+                        * (2) read: The unwritten part should be filled with 0
+                        *           If a folio does not have any buffers,
+                        *           let's returns -EAGAIN to fallback to
+                        *           per-bh IO like block_read_full_folio().
+                        */
+                       if (!folio_buffers(bh_result->b_folio)) {
+                               err = -EAGAIN;
+                               goto done;
+                       }

bh_result is set as mapped by map_bh(), should we need to clear it if return an error?

+
+                       BUG_ON(size > sb->s_blocksize);

This check is equivalent to the following condition and is not necessary.

                } else if (iblock == valid_blks &&
                           (ei->valid_size & (sb->s_blocksize - 1))) {

RE: [PATCH] exfat: fix random stack corruption after get_block

[Sungjong Seo]

Hi Yuezhang,
> Subject: Re: [PATCH] exfat: fix random stack corruption after get_block
> 
> +                       /*
> +                        * No buffer_head is allocated.
> +                        * (1) bmap: It's enough to fill bh_result without
I/O.
> +                        * (2) read: The unwritten part should be filled
with 0
> +                        *           If a folio does not have any buffers,
> +                        *           let's returns -EAGAIN to fallback to
> +                        *           per-bh IO like
block_read_full_folio().
> +                        */
> +                       if (!folio_buffers(bh_result->b_folio)) {
> +                               err = -EAGAIN;
> +                               goto done;
> +                       }
> 
> bh_result is set as mapped by map_bh(), should we need to clear it if
> return an error?
I looked a little deeper into do_mpage_readpage() and
block_read_full_folio(), and from a security perspective, it seems that
unmap is necessary in all error situations. Otherwise, unwritten areas
may be exposed.

> 
> +
> +                       BUG_ON(size > sb->s_blocksize);
> 
> This check is equivalent to the following condition and is not necessary.
> 
>                 } else if (iblock == valid_blks &&
>                            (ei->valid_size & (sb->s_blocksize - 1))) {

Yes, I think so, I'll remove it with v2.

Thanks