[PATCH fs/bfs 0/2] bfs: fix null-ptr-deref and possible warning in bfs_move_block() func
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
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,
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
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
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-... 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@i...) 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@i...)
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
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);
linux-stable-mirror@lists.linaro.org
-
kernel test robot -
kovalev＠altlinux.org -
Markus Elfring -
Василий Ковалев