| Message ID | 1a2dc8f198e1225ddd40833de76b60c7ee20d22d.1587024137.git.riteshh@linux.ibm.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [PATCHv2,1/1] ext4: Fix overflow case for map.m_len in ext4_iomap_begin_* | expand |
NACK on this patch. Even though this fixes syzcaller reproducer, still it seems the right fix should be:- 1. To make EXT4_MAX_LOGICAL_BLOCK to 0xfffffffe 2. And also add fiemap_check_ranges() call in overlayfs. This is because fiemap_check_ranges() takes care of truncating the length parameter to max sb->s_maxbytes which underlying filesystem can handle. This will be similar to how VFS calls for fiemap on underlying FS. Currently running xfstests on patches which implements above logic. -ritesh On 4/17/20 12:22 AM, Ritesh Harjani wrote: > EXT4_MAX_LOGICAL_BLOCK - map.m_lblk + 1 in case when > map.m_lblk (offset) is 0 could overflow an unsigned int > and become 0. > > Fix this. > > Fixes: d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework") > Reported-and-tested-by: syzbot+77fa5bdb65cc39711820@syzkaller.appspotmail.com > Reviewed-by: Jan Kara <jack@suse.cz> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > --- > @Jan, > I retained your Reviewed by, since there was no logic change, but just couple > of minor change - missed semicolon and tab space issue. > > fs/ext4/inode.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index e416096fc081..d9feaaad8ab8 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3424,6 +3424,7 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > int ret; > struct ext4_map_blocks map; > u8 blkbits = inode->i_blkbits; > + loff_t len; > > if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK) > return -EINVAL; > @@ -3435,8 +3436,11 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > * Calculate the first and last logical blocks respectively. > */ > map.m_lblk = offset >> blkbits; > - map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits, > - EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; > + len = min_t(loff_t, (offset + length - 1) >> blkbits, > + EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; > + if (len > EXT4_MAX_LOGICAL_BLOCK) > + len = EXT4_MAX_LOGICAL_BLOCK; > + map.m_len = len; > > if (flags & IOMAP_WRITE) > ret = ext4_iomap_alloc(inode, &map, flags); > @@ -3524,6 +3528,7 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset, > bool delalloc = false; > struct ext4_map_blocks map; > u8 blkbits = inode->i_blkbits; > + loff_t len; > > if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK) > return -EINVAL; > @@ -3541,8 +3546,11 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset, > * Calculate the first and last logical block respectively. > */ > map.m_lblk = offset >> blkbits; > - map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits, > - EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; > + len = min_t(loff_t, (offset + length - 1) >> blkbits, > + EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; > + if (len > EXT4_MAX_LOGICAL_BLOCK) > + len = EXT4_MAX_LOGICAL_BLOCK; > + map.m_len = len; > > /* > * Fiemap callers may call for offset beyond s_bitmap_maxbytes. >
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index e416096fc081..d9feaaad8ab8 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3424,6 +3424,7 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, int ret; struct ext4_map_blocks map; u8 blkbits = inode->i_blkbits; + loff_t len; if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK) return -EINVAL; @@ -3435,8 +3436,11 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, * Calculate the first and last logical blocks respectively. */ map.m_lblk = offset >> blkbits; - map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits, - EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; + len = min_t(loff_t, (offset + length - 1) >> blkbits, + EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; + if (len > EXT4_MAX_LOGICAL_BLOCK) + len = EXT4_MAX_LOGICAL_BLOCK; + map.m_len = len; if (flags & IOMAP_WRITE) ret = ext4_iomap_alloc(inode, &map, flags); @@ -3524,6 +3528,7 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset, bool delalloc = false; struct ext4_map_blocks map; u8 blkbits = inode->i_blkbits; + loff_t len; if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK) return -EINVAL; @@ -3541,8 +3546,11 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset, * Calculate the first and last logical block respectively. */ map.m_lblk = offset >> blkbits; - map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits, - EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; + len = min_t(loff_t, (offset + length - 1) >> blkbits, + EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; + if (len > EXT4_MAX_LOGICAL_BLOCK) + len = EXT4_MAX_LOGICAL_BLOCK; + map.m_len = len; /* * Fiemap callers may call for offset beyond s_bitmap_maxbytes.
EXT4_MAX_LOGICAL_BLOCK - map.m_lblk + 1 in case when map.m_lblk (offset) is 0 could overflow an unsigned int and become 0. Fix this. Fixes: d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework") Reported-and-tested-by: syzbot+77fa5bdb65cc39711820@syzkaller.appspotmail.com Reviewed-by: Jan Kara <jack@suse.cz> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> --- @Jan, I retained your Reviewed by, since there was no logic change, but just couple of minor change - missed semicolon and tab space issue. fs/ext4/inode.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)