[PATCH 1/3] xfs: ubsan fixes

[PATCH 1/3] xfs: ubsan fixes

[Darrick J. Wong]

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>
---
 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);

[PATCH 2/3] xfs: remove unused parameter from xfs_writepage_map

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The first thing that xfs_writepage_map does is clobber the offset
parameter.  Since we never use the passed-in value, turn the parameter
into a local variable.  This gets rid of an UBSAN warning in generic/466.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.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 b0cccf8..21e2d70 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -896,13 +896,13 @@ xfs_writepage_map(
 	struct writeback_control *wbc,
 	struct inode		*inode,
 	struct page		*page,
-	loff_t			offset,
-	uint64_t              end_offset)
+	uint64_t		end_offset)
 {
 	LIST_HEAD(submit_list);
 	struct xfs_ioend	*ioend, *next;
 	struct buffer_head	*bh, *head;
 	ssize_t			len = i_blocksize(inode);
+	uint64_t		offset;
 	int			error = 0;
 	int			count = 0;
 	int			uptodate = 1;
@@ -1146,7 +1146,7 @@ xfs_do_writepage(
 		end_offset = offset;
 	}
 
-	return xfs_writepage_map(wpc, wbc, inode, page, offset, end_offset);
+	return xfs_writepage_map(wpc, wbc, inode, page, end_offset);
 
 redirty:
 	redirty_page_for_writepage(wbc, page);

Re: [PATCH 2/3] xfs: remove unused parameter from xfs_writepage_map

[Brian Foster]

On Tue, Nov 28, 2017 at 09:40:03AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The first thing that xfs_writepage_map does is clobber the offset
> parameter.  Since we never use the passed-in value, turn the parameter
> into a local variable.  This gets rid of an UBSAN warning in generic/466.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

I thought Dave might have had a patch that addresses this, but no harm
in fixing it on its own:

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 b0cccf8..21e2d70 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -896,13 +896,13 @@ xfs_writepage_map(
>  	struct writeback_control *wbc,
>  	struct inode		*inode,
>  	struct page		*page,
> -	loff_t			offset,
> -	uint64_t              end_offset)
> +	uint64_t		end_offset)
>  {
>  	LIST_HEAD(submit_list);
>  	struct xfs_ioend	*ioend, *next;
>  	struct buffer_head	*bh, *head;
>  	ssize_t			len = i_blocksize(inode);
> +	uint64_t		offset;
>  	int			error = 0;
>  	int			count = 0;
>  	int			uptodate = 1;
> @@ -1146,7 +1146,7 @@ xfs_do_writepage(
>  		end_offset = offset;
>  	}
>  
> -	return xfs_writepage_map(wpc, wbc, inode, page, offset, end_offset);
> +	return xfs_writepage_map(wpc, wbc, inode, page, end_offset);
>  
>  redirty:
>  	redirty_page_for_writepage(wbc, page);
> 
> --
> 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

[PATCH 3/3] xfs: scrub inode mode properly

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Since we've used up all the bits in i_mode, the existing mode check
doesn't actually do anything useful.  However, we've not used all the
bit values in the format portion of i_mode, so we /do/ need to test
that for bad values.

Fixes: 80e4e1268 ("xfs: scrub inodes")
Fixes-coverity-id: 1423992
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/inode.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
index 637b7a8..f120fb2 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -318,8 +318,20 @@ xfs_scrub_dinode(
 
 	/* di_mode */
 	mode = be16_to_cpu(dip->di_mode);
-	if (mode & ~(S_IALLUGO | S_IFMT))
+	switch (mode & S_IFMT) {
+	case S_IFLNK:
+	case S_IFREG:
+	case S_IFDIR:
+	case S_IFCHR:
+	case S_IFBLK:
+	case S_IFIFO:
+	case S_IFSOCK:
+		/* mode is recognized */
+		break;
+	default:
 		xfs_scrub_ino_set_corrupt(sc, ino, bp);
+		break;
+	}
 
 	/* v1/v2 fields */
 	switch (dip->di_version) {

Re: [PATCH 3/3] xfs: scrub inode mode properly

[Brian Foster]

On Tue, Nov 28, 2017 at 09:40:09AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since we've used up all the bits in i_mode, the existing mode check
> doesn't actually do anything useful.  However, we've not used all the
> bit values in the format portion of i_mode, so we /do/ need to test
> that for bad values.
> 
> Fixes: 80e4e1268 ("xfs: scrub inodes")
> Fixes-coverity-id: 1423992
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/scrub/inode.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index 637b7a8..f120fb2 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -318,8 +318,20 @@ xfs_scrub_dinode(
>  
>  	/* di_mode */
>  	mode = be16_to_cpu(dip->di_mode);
> -	if (mode & ~(S_IALLUGO | S_IFMT))
> +	switch (mode & S_IFMT) {
> +	case S_IFLNK:
> +	case S_IFREG:
> +	case S_IFDIR:
> +	case S_IFCHR:
> +	case S_IFBLK:
> +	case S_IFIFO:
> +	case S_IFSOCK:
> +		/* mode is recognized */
> +		break;
> +	default:
>  		xfs_scrub_ino_set_corrupt(sc, ino, bp);
> +		break;
> +	}
>  
>  	/* v1/v2 fields */
>  	switch (dip->di_version) {
> 
> --
> 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

Re: [PATCH 1/3] xfs: ubsan fixes

[Brian Foster]

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

Re: [PATCH 1/3] xfs: ubsan fixes

[Christoph Hellwig]

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.