| Message ID | 20200103184739.26903-1-cengiz@kernel.wtf (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: btrfs: prevent unintentional int overflow | expand |
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.
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!
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.
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!
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,
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(-)