Coverity: ext4_iomap_alloc(): Integer handling issues

Coverity: ext4_iomap_alloc(): Integer handling issues

[coverity-bot]

Hello!

This is an experimental automated report about issues detected by Coverity
from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified
lines of code (noted below) that were touched by recent commits:

378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")

Coverity reported the following:

*** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
/fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
3382     	/*
3383     	 * We use i_size instead of i_disksize here because delalloc writeback
3384     	 * can complete at any point during the I/O and subsequently push the
3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
3386     	 * happening and thus expose allocated blocks to direct I/O reads.
3387     	 */
vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
3392
3393     	ret = ext4_map_blocks(handle, inode, map, m_flags);

If this is a false positive, please let us know so we can mark it as
such, or teach the Coverity rules to be smarter. If not, please make
sure fixes get into linux-next. :) For patches fixing this, please
include these lines (but double-check the "Fixes" first):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1487841 ("Integer handling issues")
Fixes: 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")


Thanks for your attention!

-- 
Coverity-bot

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Matthew Bobrowski]

On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> This is an experimental automated report about issues detected by Coverity
> from a scan of next-20191108 as part of the linux-next weekly scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
> 
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by recent commits:
> 
> 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> 
> Coverity reported the following:
> 
> *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> 3382     	/*
> 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> 3384     	 * can complete at any point during the I/O and subsequently push the
> 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> 3387     	 */
> vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;

In the event of an overflow in this specific context, I don't think it
would matter too much to be perfectly honest. If 'blkbits' were to
actually ever push out the signed integer to a value that couldn't be
represented by this data type, I would expect the resulting wrapping
behaviour to _only_ affect how filesystem blocks are allocated. In
that case, I/O workloads would behave alot differently, and at that
point I would hope that our filesystem related testing infrastructure
would pick this up before allowing anything to leak out into the
wild...

Unless my trail of thought is wrong? Happy to be corrected here and
educated on this.

/M

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Jan Kara]

On Tue 12-11-19 18:22:41, Matthew Bobrowski wrote:
> On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> > This is an experimental automated report about issues detected by Coverity
> > from a scan of next-20191108 as part of the linux-next weekly scan project:
> > https://scan.coverity.com/projects/linux-next-weekly-scan
> > 
> > You're getting this email because you were associated with the identified
> > lines of code (noted below) that were touched by recent commits:
> > 
> > 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> > 
> > Coverity reported the following:
> > 
> > *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> > 3382     	/*
> > 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> > 3384     	 * can complete at any point during the I/O and subsequently push the
> > 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> > 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> > 3387     	 */
> > vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> > 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> > 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> > 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
> 
> In the event of an overflow in this specific context, I don't think it
> would matter too much to be perfectly honest. If 'blkbits' were to
> actually ever push out the signed integer to a value that couldn't be
> represented by this data type, I would expect the resulting wrapping
> behaviour to _only_ affect how filesystem blocks are allocated. In
> that case, I/O workloads would behave alot differently, and at that
> point I would hope that our filesystem related testing infrastructure
> would pick this up before allowing anything to leak out into the
> wild...
> 
> Unless my trail of thought is wrong? Happy to be corrected here and
> educated on this.

Fully agreed. blkbits is never expected to be larger than 16 in this code.
So this is false positive.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Kees Cook]

On Tue, Nov 12, 2019 at 12:00:04PM +0100, Jan Kara wrote:
> On Tue 12-11-19 18:22:41, Matthew Bobrowski wrote:
> > On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> > > This is an experimental automated report about issues detected by Coverity
> > > from a scan of next-20191108 as part of the linux-next weekly scan project:
> > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > 
> > > You're getting this email because you were associated with the identified
> > > lines of code (noted below) that were touched by recent commits:
> > > 
> > > 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> > > 
> > > Coverity reported the following:
> > > 
> > > *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> > > 3382     	/*
> > > 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> > > 3384     	 * can complete at any point during the I/O and subsequently push the
> > > 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> > > 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> > > 3387     	 */
> > > vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> > > 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> > > 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> > > 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > > 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
> > 
> > In the event of an overflow in this specific context, I don't think it
> > would matter too much to be perfectly honest. If 'blkbits' were to
> > actually ever push out the signed integer to a value that couldn't be
> > represented by this data type, I would expect the resulting wrapping
> > behaviour to _only_ affect how filesystem blocks are allocated. In
> > that case, I/O workloads would behave alot differently, and at that
> > point I would hope that our filesystem related testing infrastructure
> > would pick this up before allowing anything to leak out into the
> > wild...
> > 
> > Unless my trail of thought is wrong? Happy to be corrected here and
> > educated on this.
> 
> Fully agreed. blkbits is never expected to be larger than 16 in this code.
> So this is false positive.

Thanks for looking into this!

Is it worth changing the type to u8 or something?

-- 
Kees Cook

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Matthew Bobrowski]

On Tue, Nov 12, 2019 at 12:56:45PM -0800, Kees Cook wrote:
> On Tue, Nov 12, 2019 at 12:00:04PM +0100, Jan Kara wrote:
> > On Tue 12-11-19 18:22:41, Matthew Bobrowski wrote:
> > > On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> > > > This is an experimental automated report about issues detected by Coverity
> > > > from a scan of next-20191108 as part of the linux-next weekly scan project:
> > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > > 
> > > > You're getting this email because you were associated with the identified
> > > > lines of code (noted below) that were touched by recent commits:
> > > > 
> > > > 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> > > > 
> > > > Coverity reported the following:
> > > > 
> > > > *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> > > > 3382     	/*
> > > > 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> > > > 3384     	 * can complete at any point during the I/O and subsequently push the
> > > > 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> > > > 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> > > > 3387     	 */
> > > > vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> > > > 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> > > > 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> > > > 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > > > 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
> > > 
> > > In the event of an overflow in this specific context, I don't think it
> > > would matter too much to be perfectly honest. If 'blkbits' were to
> > > actually ever push out the signed integer to a value that couldn't be
> > > represented by this data type, I would expect the resulting wrapping
> > > behaviour to _only_ affect how filesystem blocks are allocated. In
> > > that case, I/O workloads would behave alot differently, and at that
> > > point I would hope that our filesystem related testing infrastructure
> > > would pick this up before allowing anything to leak out into the
> > > wild...
> > > 
> > > Unless my trail of thought is wrong? Happy to be corrected here and
> > > educated on this.
> > 
> > Fully agreed. blkbits is never expected to be larger than 16 in this code.
> > So this is false positive.
> 
> Thanks for looking into this!

No problem!
 
> Is it worth changing the type to u8 or something?

'blkbits' in this case is already of data type u8, so this would
effectively be a no-op. :)

/M

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Kees Cook]

On Wed, Nov 13, 2019 at 08:28:47AM +1100, Matthew Bobrowski wrote:
> On Tue, Nov 12, 2019 at 12:56:45PM -0800, Kees Cook wrote:
> > On Tue, Nov 12, 2019 at 12:00:04PM +0100, Jan Kara wrote:
> > > On Tue 12-11-19 18:22:41, Matthew Bobrowski wrote:
> > > > On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> > > > > This is an experimental automated report about issues detected by Coverity
> > > > > from a scan of next-20191108 as part of the linux-next weekly scan project:
> > > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > > > 
> > > > > You're getting this email because you were associated with the identified
> > > > > lines of code (noted below) that were touched by recent commits:
> > > > > 
> > > > > 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> > > > > 
> > > > > Coverity reported the following:
> > > > > 
> > > > > *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > > /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> > > > > 3382     	/*
> > > > > 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> > > > > 3384     	 * can complete at any point during the I/O and subsequently push the
> > > > > 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> > > > > 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> > > > > 3387     	 */
> > > > > vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > > vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> > > > > 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> > > > > 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> > > > > 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > > > > 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
> > > > 
> > > > In the event of an overflow in this specific context, I don't think it
> > > > would matter too much to be perfectly honest. If 'blkbits' were to
> > > > actually ever push out the signed integer to a value that couldn't be
> > > > represented by this data type, I would expect the resulting wrapping
> > > > behaviour to _only_ affect how filesystem blocks are allocated. In
> > > > that case, I/O workloads would behave alot differently, and at that
> > > > point I would hope that our filesystem related testing infrastructure
> > > > would pick this up before allowing anything to leak out into the
> > > > wild...
> > > > 
> > > > Unless my trail of thought is wrong? Happy to be corrected here and
> > > > educated on this.
> > > 
> > > Fully agreed. blkbits is never expected to be larger than 16 in this code.
> > > So this is false positive.
> > 
> > Thanks for looking into this!
> 
> No problem!
>  
> > Is it worth changing the type to u8 or something?
> 
> 'blkbits' in this case is already of data type u8, so this would
> effectively be a no-op. :)

Hm, yeah. I guess Coverity doesn't see anything bounding it or i_blkbits
to 16. Would add something like this make sense just to validate
assumptions?

if (WARN_ON_ONCE(blkbits > 16))
    return -ENOSPC;

-- 
Kees Cook

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Matthew Bobrowski]

On Tue, Nov 12, 2019 at 02:17:33PM -0800, Kees Cook wrote:
> On Wed, Nov 13, 2019 at 08:28:47AM +1100, Matthew Bobrowski wrote:
> > On Tue, Nov 12, 2019 at 12:56:45PM -0800, Kees Cook wrote:
> > > On Tue, Nov 12, 2019 at 12:00:04PM +0100, Jan Kara wrote:
> > > > On Tue 12-11-19 18:22:41, Matthew Bobrowski wrote:
> > > > > On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> > > > > > This is an experimental automated report about issues detected by Coverity
> > > > > > from a scan of next-20191108 as part of the linux-next weekly scan project:
> > > > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > > > > 
> > > > > > You're getting this email because you were associated with the identified
> > > > > > lines of code (noted below) that were touched by recent commits:
> > > > > > 
> > > > > > 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> > > > > > 
> > > > > > Coverity reported the following:
> > > > > > 
> > > > > > *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > > > /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> > > > > > 3382     	/*
> > > > > > 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> > > > > > 3384     	 * can complete at any point during the I/O and subsequently push the
> > > > > > 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> > > > > > 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> > > > > > 3387     	 */
> > > > > > vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > > > vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> > > > > > 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> > > > > > 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> > > > > > 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > > > > > 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
> > > > > 
> > > > > In the event of an overflow in this specific context, I don't think it
> > > > > would matter too much to be perfectly honest. If 'blkbits' were to
> > > > > actually ever push out the signed integer to a value that couldn't be
> > > > > represented by this data type, I would expect the resulting wrapping
> > > > > behaviour to _only_ affect how filesystem blocks are allocated. In
> > > > > that case, I/O workloads would behave alot differently, and at that
> > > > > point I would hope that our filesystem related testing infrastructure
> > > > > would pick this up before allowing anything to leak out into the
> > > > > wild...
> > > > > 
> > > > > Unless my trail of thought is wrong? Happy to be corrected here and
> > > > > educated on this.
> > > > 
> > > > Fully agreed. blkbits is never expected to be larger than 16 in this code.
> > > > So this is false positive.
> > > 
> > > Thanks for looking into this!
> > 
> > No problem!
> >  
> > > Is it worth changing the type to u8 or something?
> > 
> > 'blkbits' in this case is already of data type u8, so this would
> > effectively be a no-op. :)
> 
> Hm, yeah. I guess Coverity doesn't see anything bounding it or i_blkbits
> to 16. Would add something like this make sense just to validate
> assumptions?
> 
> if (WARN_ON_ONCE(blkbits > 16))
>     return -ENOSPC;

In the realm of filesystem code, we'd lean towards using a different
error code if we went down the route of adding such a check i.e. -EIO
rather than -ENOSPC, where -ENOSPC is typically used to signify that
there's no space left on an underlying device.

That said, I still don't think this would be necessary here though...

/M

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Jan Kara]

On Tue 12-11-19 14:17:33, Kees Cook wrote:
> On Wed, Nov 13, 2019 at 08:28:47AM +1100, Matthew Bobrowski wrote:
> > On Tue, Nov 12, 2019 at 12:56:45PM -0800, Kees Cook wrote:
> > > On Tue, Nov 12, 2019 at 12:00:04PM +0100, Jan Kara wrote:
> > > > On Tue 12-11-19 18:22:41, Matthew Bobrowski wrote:
> > > > > On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> > > > > > This is an experimental automated report about issues detected by Coverity
> > > > > > from a scan of next-20191108 as part of the linux-next weekly scan project:
> > > > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > > > > 
> > > > > > You're getting this email because you were associated with the identified
> > > > > > lines of code (noted below) that were touched by recent commits:
> > > > > > 
> > > > > > 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> > > > > > 
> > > > > > Coverity reported the following:
> > > > > > 
> > > > > > *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > > > /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> > > > > > 3382     	/*
> > > > > > 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> > > > > > 3384     	 * can complete at any point during the I/O and subsequently push the
> > > > > > 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> > > > > > 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> > > > > > 3387     	 */
> > > > > > vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> > > > > > vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> > > > > > 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> > > > > > 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> > > > > > 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > > > > > 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
> > > > > 
> > > > > In the event of an overflow in this specific context, I don't think it
> > > > > would matter too much to be perfectly honest. If 'blkbits' were to
> > > > > actually ever push out the signed integer to a value that couldn't be
> > > > > represented by this data type, I would expect the resulting wrapping
> > > > > behaviour to _only_ affect how filesystem blocks are allocated. In
> > > > > that case, I/O workloads would behave alot differently, and at that
> > > > > point I would hope that our filesystem related testing infrastructure
> > > > > would pick this up before allowing anything to leak out into the
> > > > > wild...
> > > > > 
> > > > > Unless my trail of thought is wrong? Happy to be corrected here and
> > > > > educated on this.
> > > > 
> > > > Fully agreed. blkbits is never expected to be larger than 16 in this code.
> > > > So this is false positive.
> > > 
> > > Thanks for looking into this!
> > 
> > No problem!
> >  
> > > Is it worth changing the type to u8 or something?
> > 
> > 'blkbits' in this case is already of data type u8, so this would
> > effectively be a no-op. :)
> 
> Hm, yeah. I guess Coverity doesn't see anything bounding it or i_blkbits
> to 16. Would add something like this make sense just to validate
> assumptions?
> 
> if (WARN_ON_ONCE(blkbits > 16))
>     return -ENOSPC;

Well, I don't think we want to clutter various places in the code with
checks that inode->i_blkbits (which is what blkbits actually is) is what we
expect. inode->i_blkbits is initialized in fs/inode.c:inode_init_always()
from sb->s_blocksize_bits and never changed. sb->s_blocksize_bits gets set
through sb_set_blocksize().  Now it would make sense to assert in
sb_set_blocksize() that block size is in the range we expect it (currently
there's just a comment there). But then I suspect that Coverity won't be
able to carry over the limits as far as into ext4_iomap_alloc() code...
Kees?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Kees Cook]

On Wed, Nov 13, 2019 at 10:37:54AM +0100, Jan Kara wrote:
> Well, I don't think we want to clutter various places in the code with
> checks that inode->i_blkbits (which is what blkbits actually is) is what we
> expect. inode->i_blkbits is initialized in fs/inode.c:inode_init_always()
> from sb->s_blocksize_bits and never changed. sb->s_blocksize_bits gets set
> through sb_set_blocksize().  Now it would make sense to assert in
> sb_set_blocksize() that block size is in the range we expect it (currently
> there's just a comment there). But then I suspect that Coverity won't be
> able to carry over the limits as far as into ext4_iomap_alloc() code...
> Kees?

Yeah, I'm not sure it's capabilities in this regard. It's still a bit of a
black box. :) I just tend to lean toward adding asserts to code-document
value range expectations. Perhaps add the check in sb_set_blocksize()
just because it's a decent thing to test, and if Coverity doesn't notice,
that's okay -- my goal is to improve the kernel which may not always
reduce the static checker noise. :)

-- 
Kees Cook

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Jan Kara]

On Wed 13-11-19 10:38:43, Kees Cook wrote:
> On Wed, Nov 13, 2019 at 10:37:54AM +0100, Jan Kara wrote:
> > Well, I don't think we want to clutter various places in the code with
> > checks that inode->i_blkbits (which is what blkbits actually is) is what we
> > expect. inode->i_blkbits is initialized in fs/inode.c:inode_init_always()
> > from sb->s_blocksize_bits and never changed. sb->s_blocksize_bits gets set
> > through sb_set_blocksize().  Now it would make sense to assert in
> > sb_set_blocksize() that block size is in the range we expect it (currently
> > there's just a comment there). But then I suspect that Coverity won't be
> > able to carry over the limits as far as into ext4_iomap_alloc() code...
> > Kees?
> 
> Yeah, I'm not sure it's capabilities in this regard. It's still a bit of a
> black box. :) I just tend to lean toward adding asserts to code-document
> value range expectations. Perhaps add the check in sb_set_blocksize()
> just because it's a decent thing to test, and if Coverity doesn't notice,
> that's okay -- my goal is to improve the kernel which may not always
> reduce the static checker noise. :)

Now I've noticed that set_blocksize() called from sb_set_blocksize()
already has these checks. So there's nothing to add. Just Coverity is not
able to carry over those limits that far...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Kees Cook]

On Thu, Nov 14, 2019 at 09:58:12AM +0100, Jan Kara wrote:
> On Wed 13-11-19 10:38:43, Kees Cook wrote:
> > On Wed, Nov 13, 2019 at 10:37:54AM +0100, Jan Kara wrote:
> > > Well, I don't think we want to clutter various places in the code with
> > > checks that inode->i_blkbits (which is what blkbits actually is) is what we
> > > expect. inode->i_blkbits is initialized in fs/inode.c:inode_init_always()
> > > from sb->s_blocksize_bits and never changed. sb->s_blocksize_bits gets set
> > > through sb_set_blocksize().  Now it would make sense to assert in
> > > sb_set_blocksize() that block size is in the range we expect it (currently
> > > there's just a comment there). But then I suspect that Coverity won't be
> > > able to carry over the limits as far as into ext4_iomap_alloc() code...
> > > Kees?
> > 
> > Yeah, I'm not sure it's capabilities in this regard. It's still a bit of a
> > black box. :) I just tend to lean toward adding asserts to code-document
> > value range expectations. Perhaps add the check in sb_set_blocksize()
> > just because it's a decent thing to test, and if Coverity doesn't notice,
> > that's okay -- my goal is to improve the kernel which may not always
> > reduce the static checker noise. :)
> 
> Now I've noticed that set_blocksize() called from sb_set_blocksize()
> already has these checks. So there's nothing to add. Just Coverity is not
> able to carry over those limits that far...

Okay, cool. I'll mark it as such. Thanks!

-- 
Kees Cook