| Message ID | 20190222005350.7535-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Fix possible NULL pointer dereference in btrfs selftest | expand |
I'm sorry, I feel bad for passing this work on to you when you didn't introduce the problems at all. I think you're doing to the right thing to change it all to error pointers, and most of callers expect that but there are a couple that need to be changed: btrfs_test_qgroups() and run_test(). regards, dan carpenter
On 2019/2/22 下午3:00, Dan Carpenter wrote: > I'm sorry, I feel bad for passing this work on to you when you didn't > introduce the problems at all. No problem at all. Who doesn't like to send out clean up patches? > > I think you're doing to the right thing to change it all to error > pointers, and most of callers expect that but there are a couple that > need to be changed: btrfs_test_qgroups() and run_test(). Thanks for pointing them all. I'll double check the call chains in next version. Thanks, Qu > > regards, > dan carpenter > >
On Fri, Feb 22, 2019 at 08:53:50AM +0800, Qu Wenruo wrote: > When CONFIG_BTRFS_FS_RUN_SANITY_TESTS is enabled, btrfs will run > selftest at module load time. > > During selftest, we allocate extent buffer using > alloc_test_extent_buffer(), instead of alloc_test_extent_buffer(). > > The problem is, unlike alloc_extent_buffer(), > alloc_test_extent_buffer() can return NULL pointer instead of error > pointer, and callers all expect error pointer other than NULL pointer. > > So this could lead to NULL pointer dereference during selftest. > > Fix it by returning error pointer in alloc_test_extent_buffer(). > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> This patch is obsoleted by https://patchwork.kernel.org/patch/10828221/ "btrfs: extent_io: Always return error pointer for extent buffer allocation failure", right?
On 2019/3/1 上午12:02, David Sterba wrote: > On Fri, Feb 22, 2019 at 08:53:50AM +0800, Qu Wenruo wrote: >> When CONFIG_BTRFS_FS_RUN_SANITY_TESTS is enabled, btrfs will run >> selftest at module load time. >> >> During selftest, we allocate extent buffer using >> alloc_test_extent_buffer(), instead of alloc_test_extent_buffer(). >> >> The problem is, unlike alloc_extent_buffer(), >> alloc_test_extent_buffer() can return NULL pointer instead of error >> pointer, and callers all expect error pointer other than NULL pointer. >> >> So this could lead to NULL pointer dereference during selftest. >> >> Fix it by returning error pointer in alloc_test_extent_buffer(). >> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > This patch is obsoleted by https://patchwork.kernel.org/patch/10828221/ > "btrfs: extent_io: Always return error pointer for extent buffer > allocation failure", right? Yup. Thanks, Qu
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 52abe4082680..a7db78f49fdb 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -4862,12 +4862,14 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, return eb; eb = alloc_dummy_extent_buffer(fs_info, start); if (!eb) - return NULL; + return ERR_PTR(-ENOMEM); eb->fs_info = fs_info; again: ret = radix_tree_preload(GFP_NOFS); - if (ret) - goto free_eb; + if (ret) { + btrfs_release_extent_buffer(eb); + return ERR_PTR(ret); + } spin_lock(&fs_info->buffer_lock); ret = radix_tree_insert(&fs_info->buffer_radix, start >> PAGE_SHIFT, eb); @@ -4875,18 +4877,16 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, radix_tree_preload_end(); if (ret == -EEXIST) { exists = find_extent_buffer(fs_info, start); - if (exists) - goto free_eb; - else - goto again; + if (exists) { + btrfs_release_extent_buffer(eb); + return exists; + } + goto again; } check_buffer_tree_ref(eb); set_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags); return eb; -free_eb: - btrfs_release_extent_buffer(eb); - return exists; } #endif
When CONFIG_BTRFS_FS_RUN_SANITY_TESTS is enabled, btrfs will run selftest at module load time. During selftest, we allocate extent buffer using alloc_test_extent_buffer(), instead of alloc_test_extent_buffer(). The problem is, unlike alloc_extent_buffer(), alloc_test_extent_buffer() can return NULL pointer instead of error pointer, and callers all expect error pointer other than NULL pointer. So this could lead to NULL pointer dereference during selftest. Fix it by returning error pointer in alloc_test_extent_buffer(). Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)