| Message ID | 151189079681.14861.10709810493861130558.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Nov 28, 2017 at 09:39:56AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Fix some complaints from the UBSAN about signed integer addition overflows. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Seems Ok: Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_aops.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > index a3eeaba..b0cccf8 100644 > --- a/fs/xfs/xfs_aops.c > +++ b/fs/xfs/xfs_aops.c > @@ -399,7 +399,7 @@ xfs_map_blocks( > (ip->i_df.if_flags & XFS_IFEXTENTS)); > ASSERT(offset <= mp->m_super->s_maxbytes); > > - if (offset + count > mp->m_super->s_maxbytes) > + if ((xfs_ufsize_t)offset + count > mp->m_super->s_maxbytes) > count = mp->m_super->s_maxbytes - offset; > end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count); > offset_fsb = XFS_B_TO_FSBT(mp, offset); > @@ -1265,7 +1265,7 @@ xfs_map_trim_size( > if (mapping_size > size) > mapping_size = size; > if (offset < i_size_read(inode) && > - offset + mapping_size >= i_size_read(inode)) { > + (xfs_ufsize_t)offset + mapping_size >= i_size_read(inode)) { > /* limit mapping to block that spans EOF */ > mapping_size = roundup_64(i_size_read(inode) - offset, > i_blocksize(inode)); > @@ -1312,7 +1312,7 @@ xfs_get_blocks( > lockmode = xfs_ilock_data_map_shared(ip); > > ASSERT(offset <= mp->m_super->s_maxbytes); > - if (offset + size > mp->m_super->s_maxbytes) > + if ((xfs_ufsize_t)offset + size > mp->m_super->s_maxbytes) > size = mp->m_super->s_maxbytes - offset; > end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + size); > offset_fsb = XFS_B_TO_FSBT(mp, offset); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 28, 2017 at 09:39:56AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Fix some complaints from the UBSAN about signed integer addition overflows. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> I only noticed that now that it's in Linus' tree. Need to find some more time for the XFS list.. > - if (offset + count > mp->m_super->s_maxbytes) > + if ((xfs_ufsize_t)offset + count > mp->m_super->s_maxbytes) > count = mp->m_super->s_maxbytes - offset; I don't think this fix is useful in any meaninfless way. Yes, signed overflow in C is undefined, and unsigned overflow is and this will shut up UBSAN. But it doesn't solve the problem at all. Assuming we still need these checks and the VFS doesn't take care of it already (I'd need to double check) we want to truncate at s_maxbytes, and assuming s_maxbytes is close to ULLONG_MAX and count makes it overflow this will give the wrong result, as it won't cap anything. What we'd need instead would be something like: if (offset > mp->m_super->s_maxbytes - count) count = mp->m_super->s_maxbytes - offset; as that does the right thing. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index a3eeaba..b0cccf8 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -399,7 +399,7 @@ xfs_map_blocks( (ip->i_df.if_flags & XFS_IFEXTENTS)); ASSERT(offset <= mp->m_super->s_maxbytes); - if (offset + count > mp->m_super->s_maxbytes) + if ((xfs_ufsize_t)offset + count > mp->m_super->s_maxbytes) count = mp->m_super->s_maxbytes - offset; end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count); offset_fsb = XFS_B_TO_FSBT(mp, offset); @@ -1265,7 +1265,7 @@ xfs_map_trim_size( if (mapping_size > size) mapping_size = size; if (offset < i_size_read(inode) && - offset + mapping_size >= i_size_read(inode)) { + (xfs_ufsize_t)offset + mapping_size >= i_size_read(inode)) { /* limit mapping to block that spans EOF */ mapping_size = roundup_64(i_size_read(inode) - offset, i_blocksize(inode)); @@ -1312,7 +1312,7 @@ xfs_get_blocks( lockmode = xfs_ilock_data_map_shared(ip); ASSERT(offset <= mp->m_super->s_maxbytes); - if (offset + size > mp->m_super->s_maxbytes) + if ((xfs_ufsize_t)offset + size > mp->m_super->s_maxbytes) size = mp->m_super->s_maxbytes - offset; end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + size); offset_fsb = XFS_B_TO_FSBT(mp, offset);