Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()
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 > Reviewed-by: Marek Behún > Reviewed-by: Qu Wenruo Applied to u-boot/next, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()
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. sma...@vger.kernel.org regards, dan carpenter
Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()
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 Reviewed-by: Qu Wenruo --- 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()
On Wed, 26 Jul 2023 09:59:04 +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 > --- > 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 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
[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 --- 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
