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.8Hi,
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
> 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
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
> 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
© 2016 - 2025 Red Hat, Inc.