Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Aug 02, 2019 at 11:19:39AM +0200, Carlos Maiolino wrote:
> Hi Darrick.
> 
> > > +		return error;
> > > +
> > > +	block = ur_block;
> > > +	error = bmap(inode, &block);
> > > +
> > > +	if (error)
> > > +		ur_block = 0;
> > > +	else
> > > +		ur_block = block;
> > 
> > What happens if ur_block > INT_MAX?  Shouldn't we return zero (i.e.
> > error) instead of truncating the value?  Maybe the code does this
> > somewhere else?  Here seemed like the obvious place for an overflow
> > check as we go from sector_t to int.
> > 
> 
> The behavior should still be the same. It will get truncated, unfortunately. I
> don't think we can actually change this behavior and return zero instead of
> truncating it.

But that's even worse, because the programs that rely on FIBMAP will now
receive *incorrect* results that may point at a different file and
definitely do not point at the correct file block.

Note also that the iomap (and therefore xfs) implementation WARNs on
integer overflow and returns 0 (error) to prevent an incorrect access.

--D

> > --D
> > 
> > > +
> > > +	error = put_user(ur_block, p);
> > > +
> > > +	return error;
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.20.1
> > > 
> 
> -- 
> Carlos