[PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()
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)
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;
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
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
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;
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!
participants (4)
-
Dan Carpenter -
Marek Behún -
Qu Wenruo -
Tom Rini