Message ID | 20180220172307.31001-1-jack@suse.cz |
---|---|
State | Accepted, archived |
Headers | show |
Series | ext4: Fix offset overflow on 32-bit archs in ext4_iomap_begin() | expand |
On Tue, Feb 20, 2018 at 06:23:07PM +0100, Jan Kara wrote: > From: Jiri Slaby <jslaby@suse.cz> > > ext4_iomap_begin() has a bug where offset returned in the iomap > structure will be truncated to unsigned long size. On 64-bit > architectures this is fine but on 32-bit architectures obviously not. > Not many places actually use the offset stored in the iomap structure > but one of visible failures is in SEEK_HOLE / SEEK_DATA implementation. > If we create a file like: > > dd if=/dev/urandom of=file bs=1k seek=8m count=1 > > then > > lseek64("file", 0x100000000ULL, SEEK_DATA) > > wrongly returns 0x100000000 on unfixed kernel while it should return > 0x200000000. Avoid the overflow by proper type cast. > > Fixes: 545052e9e35a ("ext4: Switch to iomap for SEEK_HOLE / SEEK_DATA") > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Signed-off-by: Jan Kara <jack@suse.cz> Thanks, applied. - Ted
On Tue, Feb 20, 2018 at 06:23:07PM +0100, Jan Kara wrote: > From: Jiri Slaby <jslaby@suse.cz> > > ext4_iomap_begin() has a bug where offset returned in the iomap > structure will be truncated to unsigned long size. On 64-bit > architectures this is fine but on 32-bit architectures obviously not. > Not many places actually use the offset stored in the iomap structure > but one of visible failures is in SEEK_HOLE / SEEK_DATA implementation. > If we create a file like: > > dd if=/dev/urandom of=file bs=1k seek=8m count=1 > > then > > lseek64("file", 0x100000000ULL, SEEK_DATA) > > wrongly returns 0x100000000 on unfixed kernel while it should return > 0x200000000. Avoid the overflow by proper type cast. It looks like a good candidate for a regression test in fstests :) Thanks, Eryu > > Fixes: 545052e9e35a ("ext4: Switch to iomap for SEEK_HOLE / SEEK_DATA") > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/ext4/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index c94780075b04..c07d6456b548 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3524,7 +3524,7 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > iomap->flags |= IOMAP_F_DIRTY; > iomap->bdev = inode->i_sb->s_bdev; > iomap->dax_dev = sbi->s_daxdev; > - iomap->offset = first_block << blkbits; > + iomap->offset = (u64)first_block << blkbits; > iomap->length = (u64)map.m_len << blkbits; > > if (ret == 0) { > -- > 2.13.6 >
On Fri 23-03-18 09:27:47, Eryu Guan wrote: > On Tue, Feb 20, 2018 at 06:23:07PM +0100, Jan Kara wrote: > > From: Jiri Slaby <jslaby@suse.cz> > > > > ext4_iomap_begin() has a bug where offset returned in the iomap > > structure will be truncated to unsigned long size. On 64-bit > > architectures this is fine but on 32-bit architectures obviously not. > > Not many places actually use the offset stored in the iomap structure > > but one of visible failures is in SEEK_HOLE / SEEK_DATA implementation. > > If we create a file like: > > > > dd if=/dev/urandom of=file bs=1k seek=8m count=1 > > > > then > > > > lseek64("file", 0x100000000ULL, SEEK_DATA) > > > > wrongly returns 0x100000000 on unfixed kernel while it should return > > 0x200000000. Avoid the overflow by proper type cast. > > It looks like a good candidate for a regression test in fstests :) Actually, one of SEEK_HOLE/SEEK_DATA tests in fstests will fail because of this bug (I've checked that). Just not many people run fstests in fully 32-bit environments. Honza
On Thu, Mar 29, 2018 at 03:31:23PM +0200, Jan Kara wrote: > On Fri 23-03-18 09:27:47, Eryu Guan wrote: > > On Tue, Feb 20, 2018 at 06:23:07PM +0100, Jan Kara wrote: > > > From: Jiri Slaby <jslaby@suse.cz> > > > > > > ext4_iomap_begin() has a bug where offset returned in the iomap > > > structure will be truncated to unsigned long size. On 64-bit > > > architectures this is fine but on 32-bit architectures obviously not. > > > Not many places actually use the offset stored in the iomap structure > > > but one of visible failures is in SEEK_HOLE / SEEK_DATA implementation. > > > If we create a file like: > > > > > > dd if=/dev/urandom of=file bs=1k seek=8m count=1 > > > > > > then > > > > > > lseek64("file", 0x100000000ULL, SEEK_DATA) > > > > > > wrongly returns 0x100000000 on unfixed kernel while it should return > > > 0x200000000. Avoid the overflow by proper type cast. > > > > It looks like a good candidate for a regression test in fstests :) > > Actually, one of SEEK_HOLE/SEEK_DATA tests in fstests will fail because of > this bug (I've checked that). Just not many people run fstests in fully > 32-bit environments. Which fstest? --D > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Thu 29-03-18 08:41:23, Darrick J. Wong wrote: > On Thu, Mar 29, 2018 at 03:31:23PM +0200, Jan Kara wrote: > > On Fri 23-03-18 09:27:47, Eryu Guan wrote: > > > On Tue, Feb 20, 2018 at 06:23:07PM +0100, Jan Kara wrote: > > > > From: Jiri Slaby <jslaby@suse.cz> > > > > > > > > ext4_iomap_begin() has a bug where offset returned in the iomap > > > > structure will be truncated to unsigned long size. On 64-bit > > > > architectures this is fine but on 32-bit architectures obviously not. > > > > Not many places actually use the offset stored in the iomap structure > > > > but one of visible failures is in SEEK_HOLE / SEEK_DATA implementation. > > > > If we create a file like: > > > > > > > > dd if=/dev/urandom of=file bs=1k seek=8m count=1 > > > > > > > > then > > > > > > > > lseek64("file", 0x100000000ULL, SEEK_DATA) > > > > > > > > wrongly returns 0x100000000 on unfixed kernel while it should return > > > > 0x200000000. Avoid the overflow by proper type cast. > > > > > > It looks like a good candidate for a regression test in fstests :) > > > > Actually, one of SEEK_HOLE/SEEK_DATA tests in fstests will fail because of > > this bug (I've checked that). Just not many people run fstests in fully > > 32-bit environments. > > Which fstest? It was one of the tests using src/seek_sanity_test.c. I *think* it was generic/285 and one of huge_file_tests() there (tests 10-12). Honza
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index c94780075b04..c07d6456b548 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3524,7 +3524,7 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, iomap->flags |= IOMAP_F_DIRTY; iomap->bdev = inode->i_sb->s_bdev; iomap->dax_dev = sbi->s_daxdev; - iomap->offset = first_block << blkbits; + iomap->offset = (u64)first_block << blkbits; iomap->length = (u64)map.m_len << blkbits; if (ret == 0) {