[PATCH 0/4] fs: btrfs: coverity fixes

[PATCH 0/4] fs: btrfs: coverity fixes

[Qu Wenruo]

Most of the coverity reports are at least a good hint to us to improve
the code style.
Although there are some false alerts, but it's really hard to teach
coverity to be a real human (yet), thus fix the warning even it's just
false alerts.

Qu Wenruo (4):
  fs: btrfs: inode: handle uninitialized type before returning it
  fs: btrfs: volumes: prevent overflow for multiplying
  fs: btrfs: initialize @ret to 0 to prevent uninitialized return value
  fs: btrfs: initialize @ii in show_dir() to make coverity happy

 fs/btrfs/btrfs.c   | 4 ++--
 fs/btrfs/inode.c   | 6 +++++-
 fs/btrfs/volumes.c | 4 ++--
 3 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.29.1

[PATCH 1/4] fs: btrfs: inode: handle uninitialized type before returning it

[Qu Wenruo]

In btrfs_lookup_path() the local variable @type should always be updated
after we hit any file/dir.

But if @filename is NULL from the very beginning, then we don't
initialize it and return it directly.

To prevent such problem from happening, we initialize @type to
BTRFS_FT_UNKNOWN.
For normal execution route, it will get updated for each filename we
resolved.
Buf if we didn't find any path, we check if the type is still FT_UNKNOWN
and ret == 0. If true we know there is something wrong, just return
-EUCLEAN to inform the caller.

Reported-by: Coverity CID 312958
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ff330280e025..019d532a1a4b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -251,7 +251,7 @@ int btrfs_lookup_path(struct btrfs_root *root, u64 ino, const char *filename,
 	const char *cur = filename;
 	u64 next_ino;
 	u8 next_type;
-	u8 type;
+	u8 type = BTRFS_FT_UNKNOWN;
 	int len;
 	int ret = 0;
 
@@ -335,6 +335,10 @@ next:
 		cur += len;
 	}
 
+	/* We haven't found anything, but still get no error? */
+	if (type == BTRFS_FT_UNKNOWN && !ret)
+		ret = -EUCLEAN;
+
 	if (!ret) {
 		*root_ret = root;
 		*ino_ret = ino;
-- 
2.29.1

[PATCH 1/4] fs: btrfs: inode: handle uninitialized type before returning it

[Marek Behun]

Reviewed-by: Marek Beh?n <marek.behun@nic.cz>

[PATCH 1/4] fs: btrfs: inode: handle uninitialized type before returning it

[Tom Rini]

On Sat, Oct 31, 2020 at 09:07:49AM +0800, Qu Wenruo wrote:

> In btrfs_lookup_path() the local variable @type should always be updated
> after we hit any file/dir.
> 
> But if @filename is NULL from the very beginning, then we don't
> initialize it and return it directly.
> 
> To prevent such problem from happening, we initialize @type to
> BTRFS_FT_UNKNOWN.
> For normal execution route, it will get updated for each filename we
> resolved.
> Buf if we didn't find any path, we check if the type is still FT_UNKNOWN
> and ret == 0. If true we know there is something wrong, just return
> -EUCLEAN to inform the caller.
> 
> Reported-by: Coverity CID 312958
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Marek Beh?n <marek.behun@nic.cz>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201119/f0faa784/attachment.sig>

[PATCH 2/4] fs: btrfs: volumes: prevent overflow for multiplying

[Qu Wenruo]

In __btrfs_map_block() we do a int * int and assign it to u64.
This is not safe as the result (int * int) is still evaluated as (int)
thus it can overflow.

Convert one of the multiplier to u64 to prevent such problem.

In real world, this should not cause problem as we have device number
limit thus it won't go beyond 4G for a single stripe.

But it's harder to teach coverity about all these hidden limits, so just
fix the possible overflow.

Reported-by: Coverity CID 312957
Reported-by: Coverity CID 312948
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fcf52d4b0ff3..4aaaeab663f5 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1030,7 +1030,7 @@ again:
 	 */
 	stripe_nr = stripe_nr / map->stripe_len;
 
-	stripe_offset = stripe_nr * map->stripe_len;
+	stripe_offset = stripe_nr * (u64)map->stripe_len;
 	BUG_ON(offset < stripe_offset);
 
 	/* stripe_offset is the offset of this block in its stripe*/
@@ -1103,7 +1103,7 @@ again:
 			rot = stripe_nr % map->num_stripes;
 
 			/* Fill in the logical address of each stripe */
-			tmp = stripe_nr * nr_data_stripes(map);
+			tmp = (u64)stripe_nr * nr_data_stripes(map);
 
 			for (i = 0; i < nr_data_stripes(map); i++)
 				raid_map[(i+rot) % map->num_stripes] =
-- 
2.29.1

[PATCH 2/4] fs: btrfs: volumes: prevent overflow for multiplying

[Marek Behun]

On Sat, 31 Oct 2020 09:07:50 +0800
Qu Wenruo <wqu@suse.com> wrote:

> In real world, this should not cause problem as we have device number
> limit thus it won't go beyond 4G for a single stripe.

So we can't run into this overflow in U-Boot because only one device is
supported? But in Linux we can run into this issue?

[PATCH 2/4] fs: btrfs: volumes: prevent overflow for multiplying

[Qu Wenruo]

On 2020/11/2 ??7:02, Marek Behun wrote:
> On Sat, 31 Oct 2020 09:07:50 +0800
> Qu Wenruo <wqu@suse.com> wrote:
> 
>> In real world, this should not cause problem as we have device number
>> limit thus it won't go beyond 4G for a single stripe.
> 
> So we can't run into this overflow in U-Boot because only one device is
> supported? But in Linux we can run into this issue?
> 
In kernel, we have device number check, IIRC for default nodesize is
just several donzens of devices.
And since each stripe is only 64K fixed in btrfs, you can see it won't
go beyond 4G even in kernel.

Thanks,
Qu

[PATCH 2/4] fs: btrfs: volumes: prevent overflow for multiplying

[Qu Wenruo]

On 2020/11/2 ??8:20, Qu Wenruo wrote:
> 
> 
> On 2020/11/2 ??7:02, Marek Behun wrote:
>> On Sat, 31 Oct 2020 09:07:50 +0800
>> Qu Wenruo <wqu@suse.com> wrote:
>>
>>> In real world, this should not cause problem as we have device number
>>> limit thus it won't go beyond 4G for a single stripe.
>>
>> So we can't run into this overflow in U-Boot because only one device is
>> supported? But in Linux we can run into this issue?
>>
> In kernel, we have device number check, IIRC for default nodesize is
> just several donzens of devices.
> And since each stripe is only 64K fixed in btrfs, you can see it won't
> go beyond 4G even in kernel.

Sorry, the 64K stripe_len is only for RAID56, and for kernel, we always
use u64 for stripe_len, thus we won't hit the problem.

The problem is in btrfs-progs, where the code comes from, we use int,
other than u64.

Thanks for the hint for the root cause, would related values to be u64
for both btrfs-progs and u-boot to follow the kernel scheme.

Thanks,
Qu
> 
> Thanks,
> Qu
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201102/98fc7936/attachment.sig>

[PATCH 2/4] fs: btrfs: volumes: prevent overflow for multiplying

[Tom Rini]

On Sat, Oct 31, 2020 at 09:07:50AM +0800, Qu Wenruo wrote:

> In __btrfs_map_block() we do a int * int and assign it to u64.
> This is not safe as the result (int * int) is still evaluated as (int)
> thus it can overflow.
> 
> Convert one of the multiplier to u64 to prevent such problem.
> 
> In real world, this should not cause problem as we have device number
> limit thus it won't go beyond 4G for a single stripe.
> 
> But it's harder to teach coverity about all these hidden limits, so just
> fix the possible overflow.
> 
> Reported-by: Coverity CID 312957
> Reported-by: Coverity CID 312948
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210120/b7766dfc/attachment-0001.sig>

[PATCH 3/4] fs: btrfs: initialize @ret to 0 to prevent uninitialized return value

[Qu Wenruo]

In show_dir() if we hit a ROOT_ITEM, we can exit with uninitialized
@ret.

Fix it by initializing it to 0.

Reported-by: Coverity CID 312955
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/btrfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c
index e48972ffa21a..346b2c4341c2 100644
--- a/fs/btrfs/btrfs.c
+++ b/fs/btrfs/btrfs.c
@@ -36,7 +36,7 @@ static int show_dir(struct btrfs_root *root, struct extent_buffer *eb,
 	char *target = NULL;
 	char filetime[32];
 	time_t mtime;
-	int ret;
+	int ret = 0;
 
 	btrfs_dir_item_key_to_cpu(eb, di, &key);
 
-- 
2.29.1

[PATCH 3/4] fs: btrfs: initialize @ret to 0 to prevent uninitialized return value

[Marek Behun]

Reviewed-by: Marek Beh?n <marek.behun@nic.cz>

[PATCH 3/4] fs: btrfs: initialize @ret to 0 to prevent uninitialized return value

[Tom Rini]

On Sat, Oct 31, 2020 at 09:07:51AM +0800, Qu Wenruo wrote:

> In show_dir() if we hit a ROOT_ITEM, we can exit with uninitialized
> @ret.
> 
> Fix it by initializing it to 0.
> 
> Reported-by: Coverity CID 312955
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Marek Beh?n <marek.behun@nic.cz>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201119/48518516/attachment.sig>

[PATCH 4/4] fs: btrfs: initialize @ii in show_dir() to make coverity happy

[Qu Wenruo]

In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an
BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with
data read from disk.

We even have ASSERT() for this purpose, but unfortunately coverity can't
understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY,
previous if() branch must go to the read_extent_buffer() branch to fill
the @ii.

So to make coverity happy, just initialize the variable @ii to all zero.

Reported-by: Coverity CID 312950
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/btrfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c
index 346b2c4341c2..1abd2c291177 100644
--- a/fs/btrfs/btrfs.c
+++ b/fs/btrfs/btrfs.c
@@ -19,7 +19,7 @@ static int show_dir(struct btrfs_root *root, struct extent_buffer *eb,
 		    struct btrfs_dir_item *di)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_inode_item ii;
+	struct btrfs_inode_item ii = { 0 };
 	struct btrfs_key key;
 	static const char* dir_item_str[] = {
 		[BTRFS_FT_REG_FILE]	= "FILE",
-- 
2.29.1

[PATCH 4/4] fs: btrfs: initialize @ii in show_dir() to make coverity happy

[Marek Behun]

On Sat, 31 Oct 2020 09:07:52 +0800
Qu Wenruo <wqu@suse.com> wrote:

> In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an
> BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with
> data read from disk.
> 
> We even have ASSERT() for this purpose, but unfortunately coverity can't
> understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY,
> previous if() branch must go to the read_extent_buffer() branch to fill
> the @ii.
> 
> So to make coverity happy, just initialize the variable @ii to all zero.

WTF. If ASSERT excludes this from happening, coverity should understand
this. We live in 2020. I don't much like the idea to do things just to
get coverity happy if it can't understand and infer from things like
ASSERT.

[PATCH 4/4] fs: btrfs: initialize @ii in show_dir() to make coverity happy

[Qu Wenruo]

On 2020/11/2 ??7:06, Marek Behun wrote:
> On Sat, 31 Oct 2020 09:07:52 +0800
> Qu Wenruo <wqu@suse.com> wrote:
> 
>> In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an
>> BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with
>> data read from disk.
>>
>> We even have ASSERT() for this purpose, but unfortunately coverity can't
>> understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY,
>> previous if() branch must go to the read_extent_buffer() branch to fill
>> the @ii.
>>
>> So to make coverity happy, just initialize the variable @ii to all zero.
> 
> WTF. If ASSERT excludes this from happening, coverity should understand
> this. We live in 2020. I don't much like the idea to do things just to
> get coverity happy if it can't understand and infer from things like
> ASSERT.
> 
It's not ASSERT(), it's the previous if () branch. The ASSERT() is just to ensure we didn't miss anything.

The previous if () looks like this:
if (key.type == BTRFS_ROOT_ITEM_KEY) {
	/* @ii is not touched */
	/* Further more, in this branch type should only be FT_DIR */
} else {
	/* This includes the key.type == INODE_ITEM_KEY case */
	read_extent_buffer( &ii ); /* Here we fill &ii */
}

Then in the offending code we have:
	if (type == BTRFS_FT_CHRDEV || type == BTRFS_FT_BLKDEV) {
		ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
		/*
		 * If the type is above type, we must have key type INODE_ITEM
		 * Thus the ii must be filled.
		 */
		printf("%4llu,%5llu  ", btrfs_stack_inode_rdev(&ii) >> 20,
				btrfs_stack_inode_rdev(&ii) & 0xfffff);
	} else {
		if (key.type == BTRFS_INODE_ITEM_KEY)
			printf("%10llu  ", btrfs_stack_inode_size(&ii));
		else
			printf("%10llu  ", 0ULL);
	}

Thus I really tend to believe it's just a bug in coverity.
All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.

Thanks,
Qu

[PATCH 4/4] fs: btrfs: initialize @ii in show_dir() to make coverity happy

[Marek Behun]

On Mon, 2 Nov 2020 08:27:16 +0800
Qu Wenruo <wqu@suse.com> wrote:

> Thus I really tend to believe it's just a bug in coverity.
> All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.

If this is a bug in coverity, we should fix coverity, not add extra
code to U-Boot, even if it is just one instruction. That simply stinks
the same way like when systemd crashed when "debug" parameter was
present in /proc/cmdline, and they sent a patch to kernel which removed
the "debug" parameter, instead of fixing systemd. IMO.

Marek

[PATCH 4/4] fs: btrfs: initialize @ii in show_dir() to make coverity happy

[Qu Wenruo]

On 2020/11/2 ??3:24, Marek Behun wrote:
> On Mon, 2 Nov 2020 08:27:16 +0800
> Qu Wenruo <wqu@suse.com> wrote:
> 
>> Thus I really tend to believe it's just a bug in coverity.
>> All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.
> 
> If this is a bug in coverity, we should fix coverity, not add extra
> code to U-Boot, even if it is just one instruction. That simply stinks
> the same way like when systemd crashed when "debug" parameter was
> present in /proc/cmdline, and they sent a patch to kernel which removed
> the "debug" parameter, instead of fixing systemd. IMO.

Yep, you're right.

Please discard this patch.

Thanks,
Qu

> 
> Marek
> 

[PATCH 4/4] fs: btrfs: initialize @ii in show_dir() to make coverity happy

[Tom Rini]

On Mon, Nov 02, 2020 at 08:24:09AM +0100, Marek Behun wrote:
> On Mon, 2 Nov 2020 08:27:16 +0800
> Qu Wenruo <wqu@suse.com> wrote:
> 
> > Thus I really tend to believe it's just a bug in coverity.
> > All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.
> 
> If this is a bug in coverity, we should fix coverity, not add extra
> code to U-Boot, even if it is just one instruction. That simply stinks
> the same way like when systemd crashed when "debug" parameter was
> present in /proc/cmdline, and they sent a patch to kernel which removed
> the "debug" parameter, instead of fixing systemd. IMO.

To be clear, I am quite happy to mark as intentional any issues that are
flagged by Coverity but, well, intentional and not a problem.  We may
also benefit from adding a "modeling" file to help Coverity know when
some cases of issues are handled by us already.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201102/5afd412b/attachment.sig>