[PATCH fs/bfs 0/2] bfs: fix null-ptr-deref and possible warning in bfs_move_block() func

[PATCH fs/bfs 0/2] bfs: fix null-ptr-deref and possible warning in bfs_move_block() func

[kovalev]

https://syzkaller.appspot.com/bug?extid=d98fd19acd08b36ff422

[PATCH fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block
[PATCH fs/bfs 2/2] bfs: add buffer_uptodate check before mark_buffer_dirty

[PATCH fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block

[kovalev]

From: Vasiliy Kovalev <kovalev@altlinux.org>

Add a check to ensure 'sb_getblk' did not return NULL before copying data.

Found by Syzkaller:

KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
CPU: 1 PID: 1069 Comm: mark_buffer_dir Tainted: G W 6.10.0-un-def-alt0.rc7
RIP: 0010:bfs_get_block+0x3ab/0xe80 [bfs]
Call Trace:
<TASK>
? show_regs+0x8d/0xa0
? die_addr+0x50/0xd0
? exc_general_protection+0x148/0x220
? asm_exc_general_protection+0x22/0x30
? bfs_get_block+0x3ab/0xe80 [bfs]
? bfs_get_block+0x370/0xe80 [bfs]
? __pfx_bfs_get_block+0x10/0x10 [bfs]
__block_write_begin_int+0x4ae/0x16a0
? __pfx_bfs_get_block+0x10/0x10 [bfs]
? __pfx___block_write_begin_int+0x10/0x10
block_write_begin+0xb5/0x410
? __pfx_bfs_get_block+0x10/0x10 [bfs]
bfs_write_begin+0x32/0xe0 [bfs]
generic_perform_write+0x265/0x610
? __pfx_generic_perform_write+0x10/0x10
? generic_write_checks+0x323/0x4a0
? __pfx_generic_file_write_iter+0x10/0x10
__generic_file_write_iter+0x16a/0x1b0
generic_file_write_iter+0xf0/0x360
? __pfx_generic_file_write_iter+0x10/0x10
vfs_write+0x670/0x1120
? __pfx_vfs_write+0x10/0x10
ksys_write+0x127/0x260
? __pfx_ksys_write+0x10/0x10
do_syscall_64+0x9f/0x190
? __ct_user_enter+0x74/0xc0
? syscall_exit_to_user_mode+0xbb/0x1d0
? do_syscall_64+0xab/0x190
? ct_kernel_exit.isra.0+0xbb/0xe0
? __ct_user_enter+0x74/0xc0
? syscall_exit_to_user_mode+0xbb/0x1d0
? do_syscall_64+0xab/0x190
? ct_kernel_exit.isra.0+0xbb/0xe0
? clear_bhb_loop+0x45/0xa0
? clear_bhb_loop+0x45/0xa0
? clear_bhb_loop+0x45/0xa0
entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7f2bc708ed29

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+d98fd19acd08b36ff422@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
---
 fs/bfs/file.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/bfs/file.c b/fs/bfs/file.c
index a778411574a96b..cb41ca2a2854e4 100644
--- a/fs/bfs/file.c
+++ b/fs/bfs/file.c
@@ -35,16 +35,22 @@ static int bfs_move_block(unsigned long from, unsigned long to,
 					struct super_block *sb)
 {
 	struct buffer_head *bh, *new;
+	int err;
 
 	bh = sb_bread(sb, from);
 	if (!bh)
 		return -EIO;
 	new = sb_getblk(sb, to);
+	if (unlikely(!new)) {
+		err = -EIO;
+		goto out_err_new;
+	}
 	memcpy(new->b_data, bh->b_data, bh->b_size);
 	mark_buffer_dirty(new);
-	bforget(bh);
 	brelse(new);
-	return 0;
+out_err_new:
+	bforget(bh);
+	return err;
 }
 
 static int bfs_move_blocks(struct super_block *sb, unsigned long start,
-- 
2.33.8

Re: [PATCH fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block

[Markus Elfring]

> Add a check to ensure 'sb_getblk' did not return NULL before copying data.

Wording suggestion:
                        that a sb_getblk() call


How do you think about to use a summary phrase like
“Prevent null pointer dereference in bfs_move_block()”?


…
> +++ b/fs/bfs/file.c
> @@ -35,16 +35,22 @@ static int bfs_move_block(unsigned long from, unsigned long to,
>  					struct super_block *sb)
>  {
>  	struct buffer_head *bh, *new;
> +	int err;

Can a statement (like the following) become more appropriate for such
a function implementation?

	int ret = 0;


Regards,
Markus

Re: [PATCH fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block

[Василий Ковалев]

10.07.2024 23:09, Markus Elfring wrote:
>> Add a check to ensure 'sb_getblk' did not return NULL before copying data.
> 
> Wording suggestion:
>                          that a sb_getblk() call
> 
> 
> How do you think about to use a summary phrase like
> “Prevent null pointer dereference in bfs_move_block()”?

Ok, I'll change it in the next version:

bfs: prevent null pointer dereference in bfs_move_block()

Add a check to ensure that a sb_getblk() call did not return NULL before 
copying data.

> 
> …
>> +++ b/fs/bfs/file.c
>> @@ -35,16 +35,22 @@ static int bfs_move_block(unsigned long from, unsigned long to,
>>   					struct super_block *sb)
>>   {
>>   	struct buffer_head *bh, *new;
>> +	int err;
> 
> Can a statement (like the following) become more appropriate for such
> a function implementation?
> 
> 	int ret = 0;

Yes, thank you.

> 
> Regards,
> Markus
-- 
Regards,
Vasiliy Kovalev

Re: [fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block

[Markus Elfring]

> Add a check to ensure that a sb_getblk() call did not return NULL before copying data.

How do you think about another refinement for such a change description?

   Detect a failed sb_getblk() call (before copying data)
   so that null pointer dereferences should not happen any more.


Regards,
Markus

Re: [PATCH fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc7 next-20240711]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/kovalev-altlinux-org/bfs-fix-null-ptr-deref-in-bfs_move_block/20240711-072644
base:   linus/master
patch link:    https://lore.kernel.org/r/20240710191118.40431-2-kovalev%40altlinux.org
patch subject: [PATCH fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block
config: arm-randconfig-001-20240711 (https://download.01.org/0day-ci/archive/20240712/202407120052.Al11h5ur-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240712/202407120052.Al11h5ur-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407120052.Al11h5ur-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from fs/bfs/file.c:15:
   In file included from include/linux/buffer_head.h:12:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/arm/include/asm/cacheflush.h:10:
   In file included from include/linux/mm.h:2258:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> fs/bfs/file.c:44:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
      44 |         if (unlikely(!new)) {
         |             ^~~~~~~~~~~~~~
   include/linux/compiler.h:77:22: note: expanded from macro 'unlikely'
      77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/bfs/file.c:53:9: note: uninitialized use occurs here
      53 |         return err;
         |                ^~~
   fs/bfs/file.c:44:2: note: remove the 'if' if its condition is always true
      44 |         if (unlikely(!new)) {
         |         ^~~~~~~~~~~~~~~~~~~
   fs/bfs/file.c:38:9: note: initialize the variable 'err' to silence this warning
      38 |         int err;
         |                ^
         |                 = 0
   2 warnings generated.


vim +44 fs/bfs/file.c

    33	
    34	static int bfs_move_block(unsigned long from, unsigned long to,
    35						struct super_block *sb)
    36	{
    37		struct buffer_head *bh, *new;
    38		int err;
    39	
    40		bh = sb_bread(sb, from);
    41		if (!bh)
    42			return -EIO;
    43		new = sb_getblk(sb, to);
  > 44		if (unlikely(!new)) {
    45			err = -EIO;
    46			goto out_err_new;
    47		}
    48		memcpy(new->b_data, bh->b_data, bh->b_size);
    49		mark_buffer_dirty(new);
    50		brelse(new);
    51	out_err_new:
    52		bforget(bh);
    53		return err;
    54	}
    55	

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

[PATCH fs/bfs 2/2] bfs: add buffer_uptodate check before mark_buffer_dirty call

[kovalev]

From: Vasiliy Kovalev <kovalev@altlinux.org>

Add a check in bfs_move_block to ensure the new buffer is up-to-date
(buffer_uptodate) before calling mark_buffer_dirty.

Found by Syzkaller:

WARNING: CPU: 1 PID: 1046 at fs/buffer.c:1183 mark_buffer_dirty+0x394/0x3f0
CPU: 1 PID: 1046 Comm: mark_buffer_dir Not tainted 6.10.0-un-def-alt0.rc7.kasan
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.0-alt1 04/01/2014
RIP: 0010:mark_buffer_dirty+0x394/0x3f0
Call Trace:
<TASK>
? show_regs+0x8d/0xa0
? __warn+0xe6/0x380
? mark_buffer_dirty+0x394/0x3f0
? report_bug+0x348/0x480
? handle_bug+0x60/0xc0
? exc_invalid_op+0x13/0x50
? asm_exc_invalid_op+0x16/0x20
? mark_buffer_dirty+0x394/0x3f0
? mark_buffer_dirty+0x394/0x3f0
bfs_get_block+0x3ec/0xe80 [bfs]
? __pfx_bfs_get_block+0x10/0x10 [bfs]
__block_write_begin_int+0x4ae/0x16a0
? __pfx_bfs_get_block+0x10/0x10 [bfs]
? __pfx___block_write_begin_int+0x10/0x10
block_write_begin+0xb5/0x410
? __pfx_bfs_get_block+0x10/0x10 [bfs]
bfs_write_begin+0x32/0xe0 [bfs]
generic_perform_write+0x265/0x610
? __pfx_generic_perform_write+0x10/0x10
? generic_write_checks+0x323/0x4a0
? __pfx_generic_file_write_iter+0x10/0x10
__generic_file_write_iter+0x16a/0x1b0
generic_file_write_iter+0xf0/0x360
? __pfx_generic_file_write_iter+0x10/0x10
vfs_write+0x670/0x1120
? __pfx_vfs_write+0x10/0x10
ksys_write+0x127/0x260
? __pfx_ksys_write+0x10/0x10
do_syscall_64+0x9f/0x190
? do_syscall_64+0xab/0x190
? syscall_exit_to_user_mode+0xbb/0x1d0
? do_syscall_64+0xab/0x190
? lock_release+0x241/0x730
? __ct_user_enter+0xb3/0xc0
? __pfx_lock_release+0x10/0x10
? get_vtime_delta+0x116/0x270
? ct_kernel_exit.isra.0+0xbb/0xe0
? __ct_user_enter+0x74/0xc0
? syscall_exit_to_user_mode+0xbb/0x1d0
? do_syscall_64+0xab/0x190
? do_syscall_64+0xab/0x190
? ct_kernel_exit.isra.0+0xbb/0xe0
? clear_bhb_loop+0x45/0xa0
? clear_bhb_loop+0x45/0xa0
? clear_bhb_loop+0x45/0xa0
entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7f5bb79a4d2

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+d98fd19acd08b36ff422@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d98fd19acd08b36ff422
Cc: stable@vger.kernel.org
Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
---
 fs/bfs/file.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/bfs/file.c b/fs/bfs/file.c
index cb41ca2a2854e4..da91af8f41e097 100644
--- a/fs/bfs/file.c
+++ b/fs/bfs/file.c
@@ -45,8 +45,13 @@ static int bfs_move_block(unsigned long from, unsigned long to,
 		err = -EIO;
 		goto out_err_new;
 	}
+	if (!buffer_uptodate(new)) {
+		err = -EIO;
+		goto out_err;
+	}
 	memcpy(new->b_data, bh->b_data, bh->b_size);
 	mark_buffer_dirty(new);
+out_err:
 	brelse(new);
 out_err_new:
 	bforget(bh);
-- 
2.33.8