[PATCH] btrfs: Fix possible NULL pointer dereference in btrfs selftest

[PATCH] btrfs: Fix possible NULL pointer dereference in btrfs selftest

[Qu Wenruo]

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(-)

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
 
-- 
2.20.1

Re: [PATCH] btrfs: Fix possible NULL pointer dereference in btrfs selftest

[Dan Carpenter]

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

Re: [PATCH] btrfs: Fix possible NULL pointer dereference in btrfs selftest

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 584 bytes --]



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
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: Fix possible NULL pointer dereference in btrfs selftest

[David Sterba]

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?

Re: [PATCH] btrfs: Fix possible NULL pointer dereference in btrfs selftest

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 998 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]