[PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()

[PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()

[Dan Carpenter]

If btrfs_read_fs_root() fails with -ENOENT, then we go to the next
entry.  Fine.  But if it fails for a different reason then we need
to clean up and return an error code.  In the current code it
doesn't clean up but instead dereferences "root" and crashes.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
I didn't CC the btrfs mailing list.  Perhaps, I should have?

 fs/btrfs/subvolume.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/subvolume.c b/fs/btrfs/subvolume.c
index d446e7a2c418..68ca7e48e48e 100644
--- a/fs/btrfs/subvolume.c
+++ b/fs/btrfs/subvolume.c
@@ -199,6 +199,7 @@ static int list_subvolums(struct btrfs_fs_info *fs_info)
 			ret = PTR_ERR(root);
 			if (ret == -ENOENT)
 				goto next;
+			goto out;
 		}
 		ret = list_one_subvol(root, result);
 		if (ret < 0)
-- 
2.39.2

Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()

[Marek Behún]

On Wed, 26 Jul 2023 09:59:04 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> If btrfs_read_fs_root() fails with -ENOENT, then we go to the next
> entry.  Fine.  But if it fails for a different reason then we need
> to clean up and return an error code.  In the current code it
> doesn't clean up but instead dereferences "root" and crashes.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I didn't CC the btrfs mailing list.  Perhaps, I should have?
> 
>  fs/btrfs/subvolume.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/subvolume.c b/fs/btrfs/subvolume.c
> index d446e7a2c418..68ca7e48e48e 100644
> --- a/fs/btrfs/subvolume.c
> +++ b/fs/btrfs/subvolume.c
> @@ -199,6 +199,7 @@ static int list_subvolums(struct btrfs_fs_info *fs_info)
>  			ret = PTR_ERR(root);
>  			if (ret == -ENOENT)
>  				goto next;
> +			goto out;
>  		}
>  		ret = list_one_subvol(root, result);
>  		if (ret < 0)

Reviewed-by: Marek Behún <kabel@kernel.org>

nice catch :) Dan, I always wanted to ask, since I've seen many such
"nice catches" over different subsystems from you. Do you write some
tools to find these? Or do you use coccinelle, or something?

Marek

Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()

[Qu Wenruo]

On 2023/7/26 14:59, Dan Carpenter wrote:
> If btrfs_read_fs_root() fails with -ENOENT, then we go to the next
> entry.  Fine.  But if it fails for a different reason then we need
> to clean up and return an error code.  In the current code it
> doesn't clean up but instead dereferences "root" and crashes.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: Qu Wenruo <wqu@suse.com>

> ---
> I didn't CC the btrfs mailing list.  Perhaps, I should have?

This patch is fine. The function is specific to U-boot, and not utilized 
by kernel/btrfs-progs.

Thanks,
Qu

> 
>   fs/btrfs/subvolume.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/subvolume.c b/fs/btrfs/subvolume.c
> index d446e7a2c418..68ca7e48e48e 100644
> --- a/fs/btrfs/subvolume.c
> +++ b/fs/btrfs/subvolume.c
> @@ -199,6 +199,7 @@ static int list_subvolums(struct btrfs_fs_info *fs_info)
>   			ret = PTR_ERR(root);
>   			if (ret == -ENOENT)
>   				goto next;
> +			goto out;
>   		}
>   		ret = list_one_subvol(root, result);
>   		if (ret < 0)

Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()

[Dan Carpenter]

On Tue, Aug 01, 2023 at 11:05:11AM +0200, Marek Behún wrote:
> nice catch :) Dan, I always wanted to ask, since I've seen many such
> "nice catches" over different subsystems from you. Do you write some
> tools to find these? Or do you use coccinelle, or something?
> 

Thanks!  I'm using Smatch.

https://github.com/error27/smatch/

It's handy for me that u-boot and the Linux kernel have so much in
common and I can re-use a the kernel checks.  I had to make some changes
to Smatch to make it work but those are pushed now.

U-boot is something that Linaro cares about so if you have static
checker ideas then feel free to email the smatch mailing list.
smatch@vger.kernel.org

regards,
dan carpenter

Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On Wed, Jul 26, 2023 at 09:59:04AM +0300, Dan Carpenter wrote:

> If btrfs_read_fs_root() fails with -ENOENT, then we go to the next
> entry.  Fine.  But if it fails for a different reason then we need
> to clean up and return an error code.  In the current code it
> doesn't clean up but instead dereferences "root" and crashes.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]