Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Kees Cook <keescook@chromium.org>]

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