[PATCH] fs: btrfs: prevent unintentional int overflow

[PATCH] fs: btrfs: prevent unintentional int overflow

[Cengiz Can]

Coverity scan for 5.5.0-rc4 found a possible integer overflow in
tree-checker.c line 364.

`prev_csum_end = (prev_item_size / csumsize) * sectorsize;`

Quoting from scan results:

```
CID 1456959 Unintentional integer overflow

Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN) overflow_before_widen:
Potentially overflowing expression `prev_item_size / csumsize * sectorsize`
with type unsigned int (32 bits, unsigned) is evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of type u64.
(64 bits, unsigned).
```

Added a cast to `u64` on the left hand side of the expression.

Compiles fine on x86_64_defconfig with all btrfs config flags enabled.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---
 fs/btrfs/tree-checker.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 97f3520b8d98..9f58f07be779 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -361,7 +361,7 @@ static int check_csum_item(struct extent_buffer *leaf, struct btrfs_key *key,
 		u32 prev_item_size;
 
 		prev_item_size = btrfs_item_size_nr(leaf, slot - 1);
-		prev_csum_end = (prev_item_size / csumsize) * sectorsize;
+		prev_csum_end = (u64) (prev_item_size / csumsize) * sectorsize;
 		prev_csum_end += prev_key->offset;
 		if (prev_csum_end > key->offset) {
 			generic_err(leaf, slot - 1,
-- 
2.20.1

Re: [PATCH] fs: btrfs: prevent unintentional int overflow

[David Sterba]

On Fri, Jan 03, 2020 at 01:47:40PM -0500, Cengiz Can wrote:
> Coverity scan for 5.5.0-rc4 found a possible integer overflow in
> tree-checker.c line 364.
> 
> `prev_csum_end = (prev_item_size / csumsize) * sectorsize;`
> 
> Quoting from scan results:
> 
> ```
> CID 1456959 Unintentional integer overflow
> 
> Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN) overflow_before_widen:
> Potentially overflowing expression `prev_item_size / csumsize * sectorsize`
> with type unsigned int (32 bits, unsigned) is evaluated using 32-bit
> arithmetic, and then used in a context that expects an expression of type u64.
> (64 bits, unsigned).
> ```
> 
> Added a cast to `u64` on the left hand side of the expression.
> 
> Compiles fine on x86_64_defconfig with all btrfs config flags enabled.
> 
> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> ---
>  fs/btrfs/tree-checker.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 97f3520b8d98..9f58f07be779 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -361,7 +361,7 @@ static int check_csum_item(struct extent_buffer *leaf, struct btrfs_key *key,
>  		u32 prev_item_size;
>  
>  		prev_item_size = btrfs_item_size_nr(leaf, slot - 1);
> -		prev_csum_end = (prev_item_size / csumsize) * sectorsize;
> +		prev_csum_end = (u64) (prev_item_size / csumsize) * sectorsize;

The overflow can't happen in practice. Taking generous maximum value for
an item and sectorsize (64K) and doing the math will reach nowhere the
overflow limit for 32bit type:

64K / 4 * 64K = 1G

I'm not sure if this is worth adding the cast, or mark the coverity
issue as not important.

Re: [PATCH] fs: btrfs: prevent unintentional int overflow

[Cengiz Can]

On January 6, 2020 18:53:40 David Sterba <dsterba@suse.cz> wrote:

> The overflow can't happen in practice. Taking generous maximum value for
> an item and sectorsize (64K) and doing the math will reach nowhere the
> overflow limit for 32bit type:
>
> 64K / 4 * 64K = 1G

I didn't know that. Thanks for sharing.

> I'm not sure if this is worth adding the cast, or mark the coverity
> issue as not important.

If the cast is adding any overhead (which I don't think it does) I would 
kindly request adding it for the sake of completeness.

For example: if some newbie like me tries adding something, they would be 
warned that we should consider possible overflows and/or at least be 
careful with expressions that contain different sized integers.

If this sounds unnecessary, I will help with marking the Coverity issue as 
unimportant.

Thank you!

Re: [PATCH] fs: btrfs: prevent unintentional int overflow

[David Sterba]

On Tue, Jan 07, 2020 at 06:23:45PM +0300, Cengiz Can wrote:
> On January 6, 2020 18:53:40 David Sterba <dsterba@suse.cz> wrote:
> 
> > The overflow can't happen in practice. Taking generous maximum value for
> > an item and sectorsize (64K) and doing the math will reach nowhere the
> > overflow limit for 32bit type:
> >
> > 64K / 4 * 64K = 1G
> 
> I didn't know that. Thanks for sharing.
> 
> > I'm not sure if this is worth adding the cast, or mark the coverity
> > issue as not important.
> 
> If the cast is adding any overhead (which I don't think it does) I would 
> kindly request adding it for the sake of completeness.

It's not a runtime overhead but typecasts should not be needed in
general and when there's one it should be there for a reason. Sometimes
it's apparent and does not need a comment to explain why but otherwise I
see it as "overhead" when reading the code. Lots of calculations done in
btrfs fit perfectly to 32bit, eg. the b-tree node or page-related ones.
Where it matters is eg. conversion from/to bio::bi_sector to/from btrfs
logical addresses that are u64, where the interface type is unsigned
long and not a fixed width.

The size constraints of the variables used in the reported expression
are known to developers so I tend to think the typecast is not
necessary.  Maybe the static checker tool could be improved to know the
invariants, that are eg. verified in fs/btrfs/disk-io.c:validate_super.

Re: [PATCH] fs: btrfs: prevent unintentional int overflow

[Cengiz Can]

Hello David!

On 2020-01-07 19:01, David Sterba wrote:
> It's not a runtime overhead but typecasts should not be needed in
> general and when there's one it should be there for a reason. Sometimes
> it's apparent and does not need a comment to explain why but otherwise 
> I
> see it as "overhead" when reading the code. Lots of calculations done 
> in
> btrfs fit perfectly to 32bit, eg. the b-tree node or page-related ones.
> Where it matters is eg. conversion from/to bio::bi_sector to/from btrfs
> logical addresses that are u64, where the interface type is unsigned
> long and not a fixed width.

Thanks for sharing that. As I said, I'm relatively new to btrfs 
internals.

> The size constraints of the variables used in the reported expression
> are known to developers so I tend to think the typecast is not
> necessary.

Agreed.

> Maybe the static checker tool could be improved to know the
> invariants, that are eg. verified in fs/btrfs/disk-io.c:validate_super.

That's something that I will do some research on.

We can ignore this patch.

Thank you!

-- 
Cengiz Can
@cengiz_io