[PATCH 0/5] xfs: kasan/ubsan fixes

[PATCH 0/5] xfs: kasan/ubsan fixes

[Darrick J. Wong]

Hi all,

Following are five patches that fix KASAN/UBSAN warnings that I
encountered when running the xfs_repair fuzz tests.  The first, fourth,
and fifth patches strengthen the verifiers against bad values.  The
second patch fixes a coding flaw in the scrub code, and the third patch
teaches the directory scrubber to walk through a directory entry block
to find the entries rather than assuming that the hash tree addresses
are at all correct.

--D

[PATCH 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Darrick J. Wong]

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

Currently, we don't check sb_agblocks or sb_agblklog when we validate
the superblock, which means that we can fuzz garbage values into those
values and the mount succeeds.  This leads to all sorts of UBSAN
warnings in xfs/350 since we can then coerce other parts of xfs into
shifting by ridiculously large values.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_fs.h |    7 +++++++
 fs/xfs/libxfs/xfs_sb.c |    3 +++
 2 files changed, 10 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index fc4386a..2701ea0 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
 #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
 #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
 
+/* Limits on sb_agblocks/sb_agblklog */
+#define XFS_AG_BYTES(bblog)	((long long)BBSIZE << (bblog))
+#define XFS_AG_MIN_BYTES	((XFS_AG_BYTES(15)))	/* 16 MB */
+#define XFS_AG_MAX_BYTES	((XFS_AG_BYTES(31)))	/* 1 TB */
+#define XFS_AG_MIN_BLOCKS(blog)	(XFS_AG_MIN_BYTES >> (blog))
+#define XFS_AG_MAX_BLOCKS(blog)	((XFS_AG_MAX_BYTES - 1) >> (blog))
+
 /* keep the maximum size under 2^31 by a small amount */
 #define XFS_MAX_LOG_BYTES \
 	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 08e44a0..1f81b49 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -253,6 +253,9 @@ xfs_mount_validate_sb(
 	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
 	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
 	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
+	    sbp->sb_agblocks < XFS_AG_MIN_BLOCKS(sbp->sb_blocklog)	||
+	    sbp->sb_agblocks > XFS_AG_MAX_BLOCKS(sbp->sb_blocklog)	||
+	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
 	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||

[PATCH 2/5] xfs: don't iunlock unlocked inodes

[Darrick J. Wong]

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

Don't iunlock an unlocked inode, which can happen if the parent pointer
scrubber bails out with sc->ip unlocked while trying to grab the parent
directory inode.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/scrub.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 9c177ea..89178b9 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -193,7 +193,8 @@ xfs_scrub_teardown(
 		sc->fs_frozen = false;
 	}
 	if (sc->ip) {
-		xfs_iunlock(sc->ip, sc->ilock_flags);
+		if (sc->ilock_flags)
+			xfs_iunlock(sc->ip, sc->ilock_flags);
 		if (sc->ip != ip_in &&
 		    !xfs_internal_inum(sc->mp, sc->ip->i_ino))
 			iput(VFS_I(sc->ip));

[PATCH 3/5] xfs: directory scrubber must walk through data block to offset

[Darrick J. Wong]

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

In xfs_scrub_dir_rec, we must walk through the directory block entries
to arrive at the offset given by the hash structure.  If we blindly
trust the hash address, we can end up midway into a directory entry and
stray outside the block.  Found by lastbit fuzzing lents[3].address in
xfs/390 with KASAN enabled.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dir2.h      |    1 +
 fs/xfs/libxfs/xfs_dir2_data.c |   19 +++++++++++++++++++
 fs/xfs/scrub/dir.c            |   26 +++++++++++++++++++++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 1a8f2cf..2c77195 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -340,5 +340,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
 #define XFS_READDIR_BUFSIZE	(32768)
 
 unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
+void *xfs_dir3_data_endp(struct xfs_mount *mp, struct xfs_dir2_data_hdr *hdr);
 
 #endif	/* __XFS_DIR2_H__ */
diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 853d9ab..b87db03 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -1098,3 +1098,22 @@ xfs_dir2_data_use_free(
 	}
 	*needscanp = needscan;
 }
+
+/* Find the end of the entry data in a data/block format dir block. */
+void *
+xfs_dir3_data_endp(
+	struct xfs_mount		*mp,
+	struct xfs_dir2_data_hdr	*hdr)
+{
+	switch (hdr->magic) {
+	case cpu_to_be32(XFS_DIR3_BLOCK_MAGIC):
+	case cpu_to_be32(XFS_DIR2_BLOCK_MAGIC):
+		return xfs_dir2_block_leaf_p(
+				xfs_dir2_block_tail_p(mp->m_dir_geo, hdr));
+	case cpu_to_be32(XFS_DIR3_DATA_MAGIC):
+	case cpu_to_be32(XFS_DIR2_DATA_MAGIC):
+		return (char *)hdr + mp->m_dir_geo->blksize;
+	default:
+		return NULL;
+	}
+}
diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index f5a0d17..6acbed0 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -200,6 +200,7 @@ xfs_scrub_dir_rec(
 	struct xfs_inode		*dp = ds->dargs.dp;
 	struct xfs_dir2_data_entry	*dent;
 	struct xfs_buf			*bp;
+	char				*p, *endp;
 	xfs_ino_t			ino;
 	xfs_dablk_t			rec_bno;
 	xfs_dir2_db_t			db;
@@ -239,8 +240,31 @@ xfs_scrub_dir_rec(
 	}
 	xfs_scrub_buffer_recheck(ds->sc, bp);
 
-	/* Retrieve the entry, sanity check it, and compare hashes. */
 	dent = (struct xfs_dir2_data_entry *)(((char *)bp->b_addr) + off);
+
+	/* Make sure we got a real directory entry. */
+	p = (char *)mp->m_dir_inode_ops->data_entry_p(bp->b_addr);
+	endp = xfs_dir3_data_endp(mp, bp->b_addr);
+	while (p < endp) {
+		struct xfs_dir2_data_entry	*dep;
+		struct xfs_dir2_data_unused	*dup;
+
+		dup = (struct xfs_dir2_data_unused *)p;
+		if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
+			p += be16_to_cpu(dup->length);
+			continue;
+		}
+		dep = (struct xfs_dir2_data_entry *)p;
+		if (dep == dent)
+			break;
+		p += mp->m_dir_inode_ops->data_entsize(dep->namelen);
+	}
+	if (p == endp) {
+		xfs_scrub_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
+		goto out_relse;
+	}
+
+	/* Retrieve the entry, sanity check it, and compare hashes. */
 	ino = be64_to_cpu(dent->inumber);
 	hash = be32_to_cpu(ent->hashval);
 	tag = be16_to_cpup(dp->d_ops->data_entry_tag_p(dent));

[PATCH 4/5] xfs: attr leaf verifier needs to check for obviously bad count

[Darrick J. Wong]

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

In the attribute leaf verifier, we can check for obviously bad values of
firstused and count so that later attempts at lasthash don't run off the
end of the memory buffer.  Found by ones fuzzing hdr.count in xfs/400 with
KASAN.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 6fddce7..712d458 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk(
 
 static xfs_failaddr_t
 xfs_attr3_leaf_verify(
-	struct xfs_buf		*bp)
+	struct xfs_buf			*bp)
 {
-	struct xfs_mount	*mp = bp->b_target->bt_mount;
-	struct xfs_attr_leafblock *leaf = bp->b_addr;
-	struct xfs_perag *pag = bp->b_pag;
-	struct xfs_attr3_icleaf_hdr ichdr;
+	struct xfs_attr3_icleaf_hdr	ichdr;
+	struct xfs_mount		*mp = bp->b_target->bt_mount;
+	struct xfs_attr_leafblock	*leaf = bp->b_addr;
+	struct xfs_perag		*pag = bp->b_pag;
+	struct xfs_attr_leaf_entry	*entries;
 
 	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
@@ -282,6 +283,14 @@ xfs_attr3_leaf_verify(
 	if (pag && pag->pagf_init && ichdr.count == 0)
 		return __this_address;
 
+	if (ichdr.firstused > mp->m_attr_geo->blksize)
+		return __this_address;
+
+	entries = xfs_attr3_leaf_entryp(bp->b_addr);
+	if ((char *)&entries[ichdr.count] >=
+	    (char *)bp->b_addr + ichdr.firstused)
+		return __this_address;
+
 	/* XXX: need to range check rest of attr header values */
 	/* XXX: hash order check? */
 

[PATCH 5/5] xfs: btree format ifork loader should check for zero numrecs

[Darrick J. Wong]

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

A btree format inode fork with zero records makes no sense, so reject it
if we see it, or else we can miscalculate memory allocations.  Found by
zeroes fuzzing {a,u3}.bmbt.numrecs in xfs/{374,378,412} with KASAN.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_inode_fork.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 84eaf17..8c01dd5 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -307,6 +307,7 @@ xfs_iformat_btree(
 	 */
 	if (unlikely(XFS_IFORK_NEXTENTS(ip, whichfork) <=
 					XFS_IFORK_MAXEXT(ip, whichfork) ||
+		     nrecs == 0 ||
 		     XFS_BMDR_SPACE_CALC(nrecs) >
 					XFS_DFORK_SIZE(dip, mp, whichfork) ||
 		     XFS_IFORK_NEXTENTS(ip, whichfork) > ip->i_d.di_nblocks) ||

Re: [PATCH 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Brian Foster]

On Fri, Jan 12, 2018 at 02:04:06PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Currently, we don't check sb_agblocks or sb_agblklog when we validate
> the superblock, which means that we can fuzz garbage values into those
> values and the mount succeeds.  This leads to all sorts of UBSAN
> warnings in xfs/350 since we can then coerce other parts of xfs into
> shifting by ridiculously large values.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
>  fs/xfs/libxfs/xfs_sb.c |    3 +++
>  2 files changed, 10 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index fc4386a..2701ea0 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
>  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
>  
> +/* Limits on sb_agblocks/sb_agblklog */
> +#define XFS_AG_BYTES(bblog)	((long long)BBSIZE << (bblog))
> +#define XFS_AG_MIN_BYTES	((XFS_AG_BYTES(15)))	/* 16 MB */
> +#define XFS_AG_MAX_BYTES	((XFS_AG_BYTES(31)))	/* 1 TB */
> +#define XFS_AG_MIN_BLOCKS(blog)	(XFS_AG_MIN_BYTES >> (blog))
> +#define XFS_AG_MAX_BLOCKS(blog)	((XFS_AG_MAX_BYTES - 1) >> (blog))
> +

Hmm, we have some similar but differently named definitions just above.
E.g., XFS_MIN_AG_BLOCKS vs. XFS_AG_MIN_BLOCKS()..? I'm not sure where
the 64 block value comes from, but assuming it's valid.. perhaps we
should use that field for the block count and new ones for byte values?

Regardless of whether we want to check the block count here at all, I
think it's more straightforward (and less code) to just define two new
XFS_MIN/MAX_AG_BYTES values and then update the checks to use existing
conversions in terms of bytes (rather than obfuscate what is really a
size check behind block macros). For example, something like:

	...
	XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_MIN_AG_BYTES ||
	XFS_FSB_TO_B(mp, sbp->ag_agblocks) > XFS_MAX_AG_BYTES ||
	...

Brian

>  /* keep the maximum size under 2^31 by a small amount */
>  #define XFS_MAX_LOG_BYTES \
>  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 08e44a0..1f81b49 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
>  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
>  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
>  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> +	    sbp->sb_agblocks < XFS_AG_MIN_BLOCKS(sbp->sb_blocklog)	||
> +	    sbp->sb_agblocks > XFS_AG_MAX_BLOCKS(sbp->sb_blocklog)	||
> +	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
>  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> 
> --
> 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 2/5] xfs: don't iunlock unlocked inodes

[Brian Foster]

On Fri, Jan 12, 2018 at 02:04:12PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't iunlock an unlocked inode, which can happen if the parent pointer
> scrubber bails out with sc->ip unlocked while trying to grab the parent
> directory inode.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  fs/xfs/scrub/scrub.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 9c177ea..89178b9 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -193,7 +193,8 @@ xfs_scrub_teardown(
>  		sc->fs_frozen = false;
>  	}
>  	if (sc->ip) {
> -		xfs_iunlock(sc->ip, sc->ilock_flags);
> +		if (sc->ilock_flags)
> +			xfs_iunlock(sc->ip, sc->ilock_flags);
>  		if (sc->ip != ip_in &&
>  		    !xfs_internal_inum(sc->mp, sc->ip->i_ino))
>  			iput(VFS_I(sc->ip));
> 
> --
> 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 3/5] xfs: directory scrubber must walk through data block to offset

[Brian Foster]

On Fri, Jan 12, 2018 at 02:04:18PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_scrub_dir_rec, we must walk through the directory block entries
> to arrive at the offset given by the hash structure.  If we blindly
> trust the hash address, we can end up midway into a directory entry and
> stray outside the block.  Found by lastbit fuzzing lents[3].address in
> xfs/390 with KASAN enabled.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_dir2.h      |    1 +
>  fs/xfs/libxfs/xfs_dir2_data.c |   19 +++++++++++++++++++
>  fs/xfs/scrub/dir.c            |   26 +++++++++++++++++++++++++-
>  3 files changed, 45 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 1a8f2cf..2c77195 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -340,5 +340,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
>  #define XFS_READDIR_BUFSIZE	(32768)
>  
>  unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
> +void *xfs_dir3_data_endp(struct xfs_mount *mp, struct xfs_dir2_data_hdr *hdr);
>  
>  #endif	/* __XFS_DIR2_H__ */
...
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index f5a0d17..6acbed0 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -200,6 +200,7 @@ xfs_scrub_dir_rec(
>  	struct xfs_inode		*dp = ds->dargs.dp;
>  	struct xfs_dir2_data_entry	*dent;
>  	struct xfs_buf			*bp;
> +	char				*p, *endp;
>  	xfs_ino_t			ino;
>  	xfs_dablk_t			rec_bno;
>  	xfs_dir2_db_t			db;
> @@ -239,8 +240,31 @@ xfs_scrub_dir_rec(
>  	}
>  	xfs_scrub_buffer_recheck(ds->sc, bp);
>  
> -	/* Retrieve the entry, sanity check it, and compare hashes. */
>  	dent = (struct xfs_dir2_data_entry *)(((char *)bp->b_addr) + off);
> +
> +	/* Make sure we got a real directory entry. */
> +	p = (char *)mp->m_dir_inode_ops->data_entry_p(bp->b_addr);
> +	endp = xfs_dir3_data_endp(mp, bp->b_addr);

If this helper can return NULL perhaps we should check for that case
too? Otherwise looks fine:

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

> +	while (p < endp) {
> +		struct xfs_dir2_data_entry	*dep;
> +		struct xfs_dir2_data_unused	*dup;
> +
> +		dup = (struct xfs_dir2_data_unused *)p;
> +		if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
> +			p += be16_to_cpu(dup->length);
> +			continue;
> +		}
> +		dep = (struct xfs_dir2_data_entry *)p;
> +		if (dep == dent)
> +			break;
> +		p += mp->m_dir_inode_ops->data_entsize(dep->namelen);
> +	}
> +	if (p == endp) {
> +		xfs_scrub_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
> +		goto out_relse;
> +	}
> +
> +	/* Retrieve the entry, sanity check it, and compare hashes. */
>  	ino = be64_to_cpu(dent->inumber);
>  	hash = be32_to_cpu(ent->hashval);
>  	tag = be16_to_cpup(dp->d_ops->data_entry_tag_p(dent));
> 
> --
> 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 4/5] xfs: attr leaf verifier needs to check for obviously bad count

[Brian Foster]

On Fri, Jan 12, 2018 at 02:04:24PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In the attribute leaf verifier, we can check for obviously bad values of
> firstused and count so that later attempts at lasthash don't run off the
> end of the memory buffer.  Found by ones fuzzing hdr.count in xfs/400 with
> KASAN.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |   19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 6fddce7..712d458 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk(
>  
>  static xfs_failaddr_t
>  xfs_attr3_leaf_verify(
> -	struct xfs_buf		*bp)
> +	struct xfs_buf			*bp)
>  {
> -	struct xfs_mount	*mp = bp->b_target->bt_mount;
> -	struct xfs_attr_leafblock *leaf = bp->b_addr;
> -	struct xfs_perag *pag = bp->b_pag;
> -	struct xfs_attr3_icleaf_hdr ichdr;
> +	struct xfs_attr3_icleaf_hdr	ichdr;
> +	struct xfs_mount		*mp = bp->b_target->bt_mount;
> +	struct xfs_attr_leafblock	*leaf = bp->b_addr;
> +	struct xfs_perag		*pag = bp->b_pag;
> +	struct xfs_attr_leaf_entry	*entries;
>  
>  	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
>  
> @@ -282,6 +283,14 @@ xfs_attr3_leaf_verify(
>  	if (pag && pag->pagf_init && ichdr.count == 0)
>  		return __this_address;
>  
> +	if (ichdr.firstused > mp->m_attr_geo->blksize)
> +		return __this_address;
> +
> +	entries = xfs_attr3_leaf_entryp(bp->b_addr);
> +	if ((char *)&entries[ichdr.count] >=
> +	    (char *)bp->b_addr + ichdr.firstused)
> +		return __this_address;
> +

Hmm, this check confuses me a bit. Should we not consider the header
size in this calculation? (A comment would be useful either way..)

Brian

>  	/* XXX: need to range check rest of attr header values */
>  	/* XXX: hash order check? */
>  
> 
> --
> 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 5/5] xfs: btree format ifork loader should check for zero numrecs

[Brian Foster]

On Fri, Jan 12, 2018 at 02:04:30PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> A btree format inode fork with zero records makes no sense, so reject it
> if we see it, or else we can miscalculate memory allocations.  Found by
> zeroes fuzzing {a,u3}.bmbt.numrecs in xfs/{374,378,412} with KASAN.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  fs/xfs/libxfs/xfs_inode_fork.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 84eaf17..8c01dd5 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -307,6 +307,7 @@ xfs_iformat_btree(
>  	 */
>  	if (unlikely(XFS_IFORK_NEXTENTS(ip, whichfork) <=
>  					XFS_IFORK_MAXEXT(ip, whichfork) ||
> +		     nrecs == 0 ||
>  		     XFS_BMDR_SPACE_CALC(nrecs) >
>  					XFS_DFORK_SIZE(dip, mp, whichfork) ||
>  		     XFS_IFORK_NEXTENTS(ip, whichfork) > ip->i_d.di_nblocks) ||
> 
> --
> 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/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Darrick J. Wong]

On Mon, Jan 15, 2018 at 09:41:09AM -0500, Brian Foster wrote:
> On Fri, Jan 12, 2018 at 02:04:06PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Currently, we don't check sb_agblocks or sb_agblklog when we validate
> > the superblock, which means that we can fuzz garbage values into those
> > values and the mount succeeds.  This leads to all sorts of UBSAN
> > warnings in xfs/350 since we can then coerce other parts of xfs into
> > shifting by ridiculously large values.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
> >  fs/xfs/libxfs/xfs_sb.c |    3 +++
> >  2 files changed, 10 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index fc4386a..2701ea0 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
> >  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
> >  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
> >  
> > +/* Limits on sb_agblocks/sb_agblklog */
> > +#define XFS_AG_BYTES(bblog)	((long long)BBSIZE << (bblog))
> > +#define XFS_AG_MIN_BYTES	((XFS_AG_BYTES(15)))	/* 16 MB */
> > +#define XFS_AG_MAX_BYTES	((XFS_AG_BYTES(31)))	/* 1 TB */
> > +#define XFS_AG_MIN_BLOCKS(blog)	(XFS_AG_MIN_BYTES >> (blog))
> > +#define XFS_AG_MAX_BLOCKS(blog)	((XFS_AG_MAX_BYTES - 1) >> (blog))
> > +
> 
> Hmm, we have some similar but differently named definitions just above.
> E.g., XFS_MIN_AG_BLOCKS vs. XFS_AG_MIN_BLOCKS()..? I'm not sure where
> the 64 block value comes from, but assuming it's valid.. perhaps we
> should use that field for the block count and new ones for byte values?

This whole block of code came from xfs_multidisk.h in xfsprogs... but
yes, this naming stuff gets a bit confusing.

> Regardless of whether we want to check the block count here at all, I
> think it's more straightforward (and less code) to just define two new
> XFS_MIN/MAX_AG_BYTES values and then update the checks to use existing
> conversions in terms of bytes (rather than obfuscate what is really a
> size check behind block macros). For example, something like:
> 
> 	...
> 	XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_MIN_AG_BYTES ||
> 	XFS_FSB_TO_B(mp, sbp->ag_agblocks) > XFS_MAX_AG_BYTES ||
> 	...

Ok, I'll do that.  Thanks for the review!

--D

> Brian
> 
> >  /* keep the maximum size under 2^31 by a small amount */
> >  #define XFS_MAX_LOG_BYTES \
> >  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 08e44a0..1f81b49 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
> >  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
> >  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
> >  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> > +	    sbp->sb_agblocks < XFS_AG_MIN_BLOCKS(sbp->sb_blocklog)	||
> > +	    sbp->sb_agblocks > XFS_AG_MAX_BLOCKS(sbp->sb_blocklog)	||
> > +	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
> >  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
> >  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
> >  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> > 
> > --
> > 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

Re: [PATCH 3/5] xfs: directory scrubber must walk through data block to offset

[Darrick J. Wong]

On Mon, Jan 15, 2018 at 09:41:44AM -0500, Brian Foster wrote:
> On Fri, Jan 12, 2018 at 02:04:18PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In xfs_scrub_dir_rec, we must walk through the directory block entries
> > to arrive at the offset given by the hash structure.  If we blindly
> > trust the hash address, we can end up midway into a directory entry and
> > stray outside the block.  Found by lastbit fuzzing lents[3].address in
> > xfs/390 with KASAN enabled.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_dir2.h      |    1 +
> >  fs/xfs/libxfs/xfs_dir2_data.c |   19 +++++++++++++++++++
> >  fs/xfs/scrub/dir.c            |   26 +++++++++++++++++++++++++-
> >  3 files changed, 45 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> > index 1a8f2cf..2c77195 100644
> > --- a/fs/xfs/libxfs/xfs_dir2.h
> > +++ b/fs/xfs/libxfs/xfs_dir2.h
> > @@ -340,5 +340,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
> >  #define XFS_READDIR_BUFSIZE	(32768)
> >  
> >  unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
> > +void *xfs_dir3_data_endp(struct xfs_mount *mp, struct xfs_dir2_data_hdr *hdr);
> >  
> >  #endif	/* __XFS_DIR2_H__ */
> ...
> > diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> > index f5a0d17..6acbed0 100644
> > --- a/fs/xfs/scrub/dir.c
> > +++ b/fs/xfs/scrub/dir.c
> > @@ -200,6 +200,7 @@ xfs_scrub_dir_rec(
> >  	struct xfs_inode		*dp = ds->dargs.dp;
> >  	struct xfs_dir2_data_entry	*dent;
> >  	struct xfs_buf			*bp;
> > +	char				*p, *endp;
> >  	xfs_ino_t			ino;
> >  	xfs_dablk_t			rec_bno;
> >  	xfs_dir2_db_t			db;
> > @@ -239,8 +240,31 @@ xfs_scrub_dir_rec(
> >  	}
> >  	xfs_scrub_buffer_recheck(ds->sc, bp);
> >  
> > -	/* Retrieve the entry, sanity check it, and compare hashes. */
> >  	dent = (struct xfs_dir2_data_entry *)(((char *)bp->b_addr) + off);
> > +
> > +	/* Make sure we got a real directory entry. */
> > +	p = (char *)mp->m_dir_inode_ops->data_entry_p(bp->b_addr);
> > +	endp = xfs_dir3_data_endp(mp, bp->b_addr);
> 
> If this helper can return NULL perhaps we should check for that case
> too? Otherwise looks fine:

Yep, good catch; fixed.

--D

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > +	while (p < endp) {
> > +		struct xfs_dir2_data_entry	*dep;
> > +		struct xfs_dir2_data_unused	*dup;
> > +
> > +		dup = (struct xfs_dir2_data_unused *)p;
> > +		if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
> > +			p += be16_to_cpu(dup->length);
> > +			continue;
> > +		}
> > +		dep = (struct xfs_dir2_data_entry *)p;
> > +		if (dep == dent)
> > +			break;
> > +		p += mp->m_dir_inode_ops->data_entsize(dep->namelen);
> > +	}
> > +	if (p == endp) {
> > +		xfs_scrub_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
> > +		goto out_relse;
> > +	}
> > +
> > +	/* Retrieve the entry, sanity check it, and compare hashes. */
> >  	ino = be64_to_cpu(dent->inumber);
> >  	hash = be32_to_cpu(ent->hashval);
> >  	tag = be16_to_cpup(dp->d_ops->data_entry_tag_p(dent));
> > 
> > --
> > 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

Re: [PATCH 4/5] xfs: attr leaf verifier needs to check for obviously bad count

[Darrick J. Wong]

On Mon, Jan 15, 2018 at 09:42:12AM -0500, Brian Foster wrote:
> On Fri, Jan 12, 2018 at 02:04:24PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In the attribute leaf verifier, we can check for obviously bad values of
> > firstused and count so that later attempts at lasthash don't run off the
> > end of the memory buffer.  Found by ones fuzzing hdr.count in xfs/400 with
> > KASAN.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr_leaf.c |   19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index 6fddce7..712d458 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk(
> >  
> >  static xfs_failaddr_t
> >  xfs_attr3_leaf_verify(
> > -	struct xfs_buf		*bp)
> > +	struct xfs_buf			*bp)
> >  {
> > -	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > -	struct xfs_attr_leafblock *leaf = bp->b_addr;
> > -	struct xfs_perag *pag = bp->b_pag;
> > -	struct xfs_attr3_icleaf_hdr ichdr;
> > +	struct xfs_attr3_icleaf_hdr	ichdr;
> > +	struct xfs_mount		*mp = bp->b_target->bt_mount;
> > +	struct xfs_attr_leafblock	*leaf = bp->b_addr;
> > +	struct xfs_perag		*pag = bp->b_pag;
> > +	struct xfs_attr_leaf_entry	*entries;
> >  
> >  	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
> >  
> > @@ -282,6 +283,14 @@ xfs_attr3_leaf_verify(
> >  	if (pag && pag->pagf_init && ichdr.count == 0)
> >  		return __this_address;
> >  
> > +	if (ichdr.firstused > mp->m_attr_geo->blksize)
> > +		return __this_address;
> > +
> > +	entries = xfs_attr3_leaf_entryp(bp->b_addr);
> > +	if ((char *)&entries[ichdr.count] >=
> > +	    (char *)bp->b_addr + ichdr.firstused)
> > +		return __this_address;
> > +
> 
> Hmm, this check confuses me a bit. Should we not consider the header
> size in this calculation? (A comment would be useful either way..)

firstused is the offset in the block, not counting the header.
I will improve the comment (and the checks) in this manner:

/*
 * firstused is the block offset of the first name info structure.
 * Make sure it doesn't go off the block or crash into the header.
 */
if (ichdr.firstused > mp->m_attr_geo->blksize)
	return __this_address;
if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf))
	return __this_address;

/* Make sure the entries array doesn't crash into the name info. */
entries = xfs_attr3_leaf_entryp(bp->b_addr);
if ((char *)&entries[ichdr.count] >=
    (char *)bp->b_addr + ichdr.firstused)
	return __this_address;

--D

> Brian
> 
> >  	/* XXX: need to range check rest of attr header values */
> >  	/* XXX: hash order check? */
> >  
> > 
> > --
> > 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

[PATCH v2 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Darrick J. Wong]

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

Currently, we don't check sb_agblocks or sb_agblklog when we validate
the superblock, which means that we can fuzz garbage values into those
values and the mount succeeds.  This leads to all sorts of UBSAN
warnings in xfs/350 since we can then coerce other parts of xfs into
shifting by ridiculously large values.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: simplify ag min/max size definitions
---
 fs/xfs/libxfs/xfs_fs.h |    7 +++++++
 fs/xfs/libxfs/xfs_sb.c |    3 +++
 2 files changed, 10 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index fc4386a..3ab1870 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
 #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
 #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
 
+/*
+ * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
+ * 16MB or larger than 1TB.
+ */
+#define XFS_AG_MIN_BYTES	(1ULL << 24)	/* 16 MB */
+#define XFS_AG_MAX_BYTES	(1ULL << 40)	/* 1 TB */
+
 /* keep the maximum size under 2^31 by a small amount */
 #define XFS_MAX_LOG_BYTES \
 	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 08e44a0..bdb4f74 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -253,6 +253,9 @@ xfs_mount_validate_sb(
 	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
 	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
 	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
+	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_AG_MIN_BYTES	||
+	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_AG_MAX_BYTES	||
+	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
 	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||

[PATCH v2 3/5] xfs: directory scrubber must walk through data block to offset

[Darrick J. Wong]

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

In xfs_scrub_dir_rec, we must walk through the directory block entries
to arrive at the offset given by the hash structure.  If we blindly
trust the hash address, we can end up midway into a directory entry and
stray outside the block.  Found by lastbit fuzzing lents[3].address in
xfs/390 with KASAN enabled.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
v2: improve defensive pointer checking (endp theoretically can be null)
---
 fs/xfs/libxfs/xfs_dir2.h      |    1 +
 fs/xfs/libxfs/xfs_dir2_data.c |   19 +++++++++++++++++++
 fs/xfs/scrub/dir.c            |   30 +++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 1a8f2cf..2c77195 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -340,5 +340,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
 #define XFS_READDIR_BUFSIZE	(32768)
 
 unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
+void *xfs_dir3_data_endp(struct xfs_mount *mp, struct xfs_dir2_data_hdr *hdr);
 
 #endif	/* __XFS_DIR2_H__ */
diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 853d9ab..b87db03 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -1098,3 +1098,22 @@ xfs_dir2_data_use_free(
 	}
 	*needscanp = needscan;
 }
+
+/* Find the end of the entry data in a data/block format dir block. */
+void *
+xfs_dir3_data_endp(
+	struct xfs_mount		*mp,
+	struct xfs_dir2_data_hdr	*hdr)
+{
+	switch (hdr->magic) {
+	case cpu_to_be32(XFS_DIR3_BLOCK_MAGIC):
+	case cpu_to_be32(XFS_DIR2_BLOCK_MAGIC):
+		return xfs_dir2_block_leaf_p(
+				xfs_dir2_block_tail_p(mp->m_dir_geo, hdr));
+	case cpu_to_be32(XFS_DIR3_DATA_MAGIC):
+	case cpu_to_be32(XFS_DIR2_DATA_MAGIC):
+		return (char *)hdr + mp->m_dir_geo->blksize;
+	default:
+		return NULL;
+	}
+}
diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index f5a0d17..106813f 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -200,6 +200,7 @@ xfs_scrub_dir_rec(
 	struct xfs_inode		*dp = ds->dargs.dp;
 	struct xfs_dir2_data_entry	*dent;
 	struct xfs_buf			*bp;
+	char				*p, *endp;
 	xfs_ino_t			ino;
 	xfs_dablk_t			rec_bno;
 	xfs_dir2_db_t			db;
@@ -239,8 +240,35 @@ xfs_scrub_dir_rec(
 	}
 	xfs_scrub_buffer_recheck(ds->sc, bp);
 
-	/* Retrieve the entry, sanity check it, and compare hashes. */
 	dent = (struct xfs_dir2_data_entry *)(((char *)bp->b_addr) + off);
+
+	/* Make sure we got a real directory entry. */
+	p = (char *)mp->m_dir_inode_ops->data_entry_p(bp->b_addr);
+	endp = xfs_dir3_data_endp(mp, bp->b_addr);
+	if (!endp) {
+		xfs_scrub_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
+		goto out_relse;
+	}
+	while (p < endp) {
+		struct xfs_dir2_data_entry	*dep;
+		struct xfs_dir2_data_unused	*dup;
+
+		dup = (struct xfs_dir2_data_unused *)p;
+		if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
+			p += be16_to_cpu(dup->length);
+			continue;
+		}
+		dep = (struct xfs_dir2_data_entry *)p;
+		if (dep == dent)
+			break;
+		p += mp->m_dir_inode_ops->data_entsize(dep->namelen);
+	}
+	if (p >= endp) {
+		xfs_scrub_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
+		goto out_relse;
+	}
+
+	/* Retrieve the entry, sanity check it, and compare hashes. */
 	ino = be64_to_cpu(dent->inumber);
 	hash = be32_to_cpu(ent->hashval);
 	tag = be16_to_cpup(dp->d_ops->data_entry_tag_p(dent));

[PATCH v2 4/5] xfs: attr leaf verifier needs to check for obviously bad count

[Darrick J. Wong]

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

In the attribute leaf verifier, we can check for obviously bad values of
firstused and count so that later attempts at lasthash don't run off the
end of the memory buffer.  Found by ones fuzzing hdr.count in xfs/400 with
KASAN.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: strengthen checking, clarify what we're testing via comments
---
 fs/xfs/libxfs/xfs_attr_leaf.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 6fddce7..f1a1c60 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk(
 
 static xfs_failaddr_t
 xfs_attr3_leaf_verify(
-	struct xfs_buf		*bp)
+	struct xfs_buf			*bp)
 {
-	struct xfs_mount	*mp = bp->b_target->bt_mount;
-	struct xfs_attr_leafblock *leaf = bp->b_addr;
-	struct xfs_perag *pag = bp->b_pag;
-	struct xfs_attr3_icleaf_hdr ichdr;
+	struct xfs_attr3_icleaf_hdr	ichdr;
+	struct xfs_mount		*mp = bp->b_target->bt_mount;
+	struct xfs_attr_leafblock	*leaf = bp->b_addr;
+	struct xfs_perag		*pag = bp->b_pag;
+	struct xfs_attr_leaf_entry	*entries;
 
 	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
@@ -282,6 +283,21 @@ xfs_attr3_leaf_verify(
 	if (pag && pag->pagf_init && ichdr.count == 0)
 		return __this_address;
 
+	/*
+	 * firstused is the block offset of the first name info structure.
+	 * Make sure it doesn't go off the block or crash into the header.
+	 */
+	if (ichdr.firstused > mp->m_attr_geo->blksize)
+		return __this_address;
+	if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf))
+		return __this_address;
+
+	/* Make sure the entries array doesn't crash into the name info. */
+	entries = xfs_attr3_leaf_entryp(bp->b_addr);
+	if ((char *)&entries[ichdr.count] >=
+	    (char *)bp->b_addr + ichdr.firstused)
+		return __this_address;
+
 	/* XXX: need to range check rest of attr header values */
 	/* XXX: hash order check? */
 

Re: [PATCH v2 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Dave Chinner]

On Mon, Jan 15, 2018 at 12:03:13PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Currently, we don't check sb_agblocks or sb_agblklog when we validate
> the superblock, which means that we can fuzz garbage values into those
> values and the mount succeeds.  This leads to all sorts of UBSAN
> warnings in xfs/350 since we can then coerce other parts of xfs into
> shifting by ridiculously large values.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: simplify ag min/max size definitions
> ---
>  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
>  fs/xfs/libxfs/xfs_sb.c |    3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index fc4386a..3ab1870 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
>  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
>  
> +/*
> + * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
> + * 16MB or larger than 1TB.
> + */
> +#define XFS_AG_MIN_BYTES	(1ULL << 24)	/* 16 MB */
> +#define XFS_AG_MAX_BYTES	(1ULL << 40)	/* 1 TB */

Sure, but....
> +
>  /* keep the maximum size under 2^31 by a small amount */
>  #define XFS_MAX_LOG_BYTES \
>  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 08e44a0..bdb4f74 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
>  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
>  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
>  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_AG_MIN_BYTES	||
> +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_AG_MAX_BYTES	||

.... this really needs to be checked against sb_dblocks /
sb_agcount. i.e. check against current filesystem size and geometry,
not against the theoretical maximum.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 3/5] xfs: directory scrubber must walk through data block to offset

[Dave Chinner]

On Mon, Jan 15, 2018 at 12:04:47PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_scrub_dir_rec, we must walk through the directory block entries
> to arrive at the offset given by the hash structure.  If we blindly
> trust the hash address, we can end up midway into a directory entry and
> stray outside the block.  Found by lastbit fuzzing lents[3].address in
> xfs/390 with KASAN enabled.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
> v2: improve defensive pointer checking (endp theoretically can be null)
> ---
>  fs/xfs/libxfs/xfs_dir2.h      |    1 +
>  fs/xfs/libxfs/xfs_dir2_data.c |   19 +++++++++++++++++++
>  fs/xfs/scrub/dir.c            |   30 +++++++++++++++++++++++++++++-
>  3 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 1a8f2cf..2c77195 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -340,5 +340,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
>  #define XFS_READDIR_BUFSIZE	(32768)
>  
>  unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
> +void *xfs_dir3_data_endp(struct xfs_mount *mp, struct xfs_dir2_data_hdr *hdr);
>  
>  #endif	/* __XFS_DIR2_H__ */
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index 853d9ab..b87db03 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -1098,3 +1098,22 @@ xfs_dir2_data_use_free(
>  	}
>  	*needscanp = needscan;
>  }
> +
> +/* Find the end of the entry data in a data/block format dir block. */
> +void *
> +xfs_dir3_data_endp(
> +	struct xfs_mount		*mp,
> +	struct xfs_dir2_data_hdr	*hdr)
> +{
> +	switch (hdr->magic) {
> +	case cpu_to_be32(XFS_DIR3_BLOCK_MAGIC):
> +	case cpu_to_be32(XFS_DIR2_BLOCK_MAGIC):
> +		return xfs_dir2_block_leaf_p(
> +				xfs_dir2_block_tail_p(mp->m_dir_geo, hdr));
> +	case cpu_to_be32(XFS_DIR3_DATA_MAGIC):
> +	case cpu_to_be32(XFS_DIR2_DATA_MAGIC):
> +		return (char *)hdr + mp->m_dir_geo->blksize;
> +	default:
> +		return NULL;
> +	}
> +}

Ok, this pattern is used in a few places. e.g.
xfs_dir2_data_make_free, __xfs_dir3_data_check and
xfs_dir2_data_freescan_int, so if you're going to add a helper, at
least convert all the other places there is the same code over to
use it...

Also, it should be passed a geo struct, not a xfs_mount.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Darrick J. Wong]

On Tue, Jan 16, 2018 at 08:31:02AM +1100, Dave Chinner wrote:
> On Mon, Jan 15, 2018 at 12:03:13PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Currently, we don't check sb_agblocks or sb_agblklog when we validate
> > the superblock, which means that we can fuzz garbage values into those
> > values and the mount succeeds.  This leads to all sorts of UBSAN
> > warnings in xfs/350 since we can then coerce other parts of xfs into
> > shifting by ridiculously large values.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: simplify ag min/max size definitions
> > ---
> >  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
> >  fs/xfs/libxfs/xfs_sb.c |    3 +++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index fc4386a..3ab1870 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
> >  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
> >  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
> >  
> > +/*
> > + * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
> > + * 16MB or larger than 1TB.
> > + */
> > +#define XFS_AG_MIN_BYTES	(1ULL << 24)	/* 16 MB */
> > +#define XFS_AG_MAX_BYTES	(1ULL << 40)	/* 1 TB */
> 
> Sure, but....
> > +
> >  /* keep the maximum size under 2^31 by a small amount */
> >  #define XFS_MAX_LOG_BYTES \
> >  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 08e44a0..bdb4f74 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
> >  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
> >  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
> >  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_AG_MIN_BYTES	||
> > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_AG_MAX_BYTES	||
> 
> .... this really needs to be checked against sb_dblocks /
> sb_agcount. i.e. check against current filesystem size and geometry,
> not against the theoretical maximum.

I think I prefer to check the theoretical maximum /and/ whether or not
ceil(dblocks/agblocks) == agcount, since we'd have to check agblocks != 0
prior to doing that anyway.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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 v2 3/5] xfs: directory scrubber must walk through data block to offset

[Darrick J. Wong]

On Tue, Jan 16, 2018 at 08:56:48AM +1100, Dave Chinner wrote:
> On Mon, Jan 15, 2018 at 12:04:47PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In xfs_scrub_dir_rec, we must walk through the directory block entries
> > to arrive at the offset given by the hash structure.  If we blindly
> > trust the hash address, we can end up midway into a directory entry and
> > stray outside the block.  Found by lastbit fuzzing lents[3].address in
> > xfs/390 with KASAN enabled.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > ---
> > v2: improve defensive pointer checking (endp theoretically can be null)
> > ---
> >  fs/xfs/libxfs/xfs_dir2.h      |    1 +
> >  fs/xfs/libxfs/xfs_dir2_data.c |   19 +++++++++++++++++++
> >  fs/xfs/scrub/dir.c            |   30 +++++++++++++++++++++++++++++-
> >  3 files changed, 49 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> > index 1a8f2cf..2c77195 100644
> > --- a/fs/xfs/libxfs/xfs_dir2.h
> > +++ b/fs/xfs/libxfs/xfs_dir2.h
> > @@ -340,5 +340,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
> >  #define XFS_READDIR_BUFSIZE	(32768)
> >  
> >  unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
> > +void *xfs_dir3_data_endp(struct xfs_mount *mp, struct xfs_dir2_data_hdr *hdr);
> >  
> >  #endif	/* __XFS_DIR2_H__ */
> > diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> > index 853d9ab..b87db03 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_data.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> > @@ -1098,3 +1098,22 @@ xfs_dir2_data_use_free(
> >  	}
> >  	*needscanp = needscan;
> >  }
> > +
> > +/* Find the end of the entry data in a data/block format dir block. */
> > +void *
> > +xfs_dir3_data_endp(
> > +	struct xfs_mount		*mp,
> > +	struct xfs_dir2_data_hdr	*hdr)
> > +{
> > +	switch (hdr->magic) {
> > +	case cpu_to_be32(XFS_DIR3_BLOCK_MAGIC):
> > +	case cpu_to_be32(XFS_DIR2_BLOCK_MAGIC):
> > +		return xfs_dir2_block_leaf_p(
> > +				xfs_dir2_block_tail_p(mp->m_dir_geo, hdr));
> > +	case cpu_to_be32(XFS_DIR3_DATA_MAGIC):
> > +	case cpu_to_be32(XFS_DIR2_DATA_MAGIC):
> > +		return (char *)hdr + mp->m_dir_geo->blksize;
> > +	default:
> > +		return NULL;
> > +	}
> > +}
> 
> Ok, this pattern is used in a few places. e.g.
> xfs_dir2_data_make_free, __xfs_dir3_data_check and
> xfs_dir2_data_freescan_int, so if you're going to add a helper, at
> least convert all the other places there is the same code over to
> use it...
> 
> Also, it should be passed a geo struct, not a xfs_mount.

Ok.

--D

> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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 v2 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Brian Foster]

On Mon, Jan 15, 2018 at 12:03:13PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Currently, we don't check sb_agblocks or sb_agblklog when we validate
> the superblock, which means that we can fuzz garbage values into those
> values and the mount succeeds.  This leads to all sorts of UBSAN
> warnings in xfs/350 since we can then coerce other parts of xfs into
> shifting by ridiculously large values.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: simplify ag min/max size definitions
> ---
>  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
>  fs/xfs/libxfs/xfs_sb.c |    3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index fc4386a..3ab1870 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
>  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
>  
> +/*
> + * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
> + * 16MB or larger than 1TB.
> + */
> +#define XFS_AG_MIN_BYTES	(1ULL << 24)	/* 16 MB */
> +#define XFS_AG_MAX_BYTES	(1ULL << 40)	/* 1 TB */
> +

Dave's comment aside... just a nit: the definitions above use
XFS_[MIN|MAX]_* naming rather than XFS_AG_*. It would be nice to be
consistent there if we stick with these.

Brian

>  /* keep the maximum size under 2^31 by a small amount */
>  #define XFS_MAX_LOG_BYTES \
>  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 08e44a0..bdb4f74 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
>  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
>  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
>  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_AG_MIN_BYTES	||
> +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_AG_MAX_BYTES	||
> +	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
>  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> --
> 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 v2 4/5] xfs: attr leaf verifier needs to check for obviously bad count

[Brian Foster]

On Mon, Jan 15, 2018 at 12:05:27PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In the attribute leaf verifier, we can check for obviously bad values of
> firstused and count so that later attempts at lasthash don't run off the
> end of the memory buffer.  Found by ones fuzzing hdr.count in xfs/400 with
> KASAN.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: strengthen checking, clarify what we're testing via comments
> ---

Ok, digging more into the code I see that firstused refers to the lowest
->nameidx of the xfs_attr_leaf_entry's in the block. On insert,
->nameidx is constructed from freemap.base, which starts at just beyond
the block header. freemap.base+freemap.size essentially refers to a raw
block offset where the name/value info is placed, so I think I just
misread that code the first time around. This makes sense and the
cleanups look good. Thanks for the clarification.

One more small question..

>  fs/xfs/libxfs/xfs_attr_leaf.c |   26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 6fddce7..f1a1c60 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk(
>  
>  static xfs_failaddr_t
>  xfs_attr3_leaf_verify(
> -	struct xfs_buf		*bp)
> +	struct xfs_buf			*bp)
>  {
> -	struct xfs_mount	*mp = bp->b_target->bt_mount;
> -	struct xfs_attr_leafblock *leaf = bp->b_addr;
> -	struct xfs_perag *pag = bp->b_pag;
> -	struct xfs_attr3_icleaf_hdr ichdr;
> +	struct xfs_attr3_icleaf_hdr	ichdr;
> +	struct xfs_mount		*mp = bp->b_target->bt_mount;
> +	struct xfs_attr_leafblock	*leaf = bp->b_addr;
> +	struct xfs_perag		*pag = bp->b_pag;
> +	struct xfs_attr_leaf_entry	*entries;
>  
>  	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
>  
> @@ -282,6 +283,21 @@ xfs_attr3_leaf_verify(
>  	if (pag && pag->pagf_init && ichdr.count == 0)
>  		return __this_address;
>  
> +	/*
> +	 * firstused is the block offset of the first name info structure.
> +	 * Make sure it doesn't go off the block or crash into the header.
> +	 */
> +	if (ichdr.firstused > mp->m_attr_geo->blksize)
> +		return __this_address;
> +	if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf))
> +		return __this_address;
> +
> +	/* Make sure the entries array doesn't crash into the name info. */
> +	entries = xfs_attr3_leaf_entryp(bp->b_addr);
> +	if ((char *)&entries[ichdr.count] >=
> +	    (char *)bp->b_addr + ichdr.firstused)
> +		return __this_address;
> +

Can the end of the entries list align with the first namelist object if
the block is full, for example? E.g., is '&entries[ichdr.count] ==
bp->b_addr + ichdr.firstused' a sane possibility?

That aside, this looks good to me:

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

>  	/* XXX: need to range check rest of attr header values */
>  	/* XXX: hash order check? */
>  
> --
> 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 v2 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Darrick J. Wong]

On Tue, Jan 16, 2018 at 07:48:46AM -0500, Brian Foster wrote:
> On Mon, Jan 15, 2018 at 12:03:13PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Currently, we don't check sb_agblocks or sb_agblklog when we validate
> > the superblock, which means that we can fuzz garbage values into those
> > values and the mount succeeds.  This leads to all sorts of UBSAN
> > warnings in xfs/350 since we can then coerce other parts of xfs into
> > shifting by ridiculously large values.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: simplify ag min/max size definitions
> > ---
> >  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
> >  fs/xfs/libxfs/xfs_sb.c |    3 +++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index fc4386a..3ab1870 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
> >  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
> >  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
> >  
> > +/*
> > + * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
> > + * 16MB or larger than 1TB.
> > + */
> > +#define XFS_AG_MIN_BYTES	(1ULL << 24)	/* 16 MB */
> > +#define XFS_AG_MAX_BYTES	(1ULL << 40)	/* 1 TB */
> > +
> 
> Dave's comment aside... just a nit: the definitions above use
> XFS_[MIN|MAX]_* naming rather than XFS_AG_*. It would be nice to be
> consistent there if we stick with these.

Since I was extracting the constants from xfs_multidisk.h I thought it
important to maintain the symbolic name to avoid breaking userspace.
I guess we /could/ just add XFS_MIN_AG_BYTES to libxfs and change
xfs_multidisk.h to do:

#define XFS_AG_MIN_BYTES	(XFS_MIN_AG_BYTES)

Or just decide that we don't care since we never install that header to
/usr/include ?

--D

> Brian
> 
> >  /* keep the maximum size under 2^31 by a small amount */
> >  #define XFS_MAX_LOG_BYTES \
> >  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 08e44a0..bdb4f74 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
> >  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
> >  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
> >  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_AG_MIN_BYTES	||
> > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_AG_MAX_BYTES	||
> > +	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
> >  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
> >  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
> >  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> > --
> > 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

Re: [PATCH v2 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Brian Foster]

On Tue, Jan 16, 2018 at 09:34:32AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 16, 2018 at 07:48:46AM -0500, Brian Foster wrote:
> > On Mon, Jan 15, 2018 at 12:03:13PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Currently, we don't check sb_agblocks or sb_agblklog when we validate
> > > the superblock, which means that we can fuzz garbage values into those
> > > values and the mount succeeds.  This leads to all sorts of UBSAN
> > > warnings in xfs/350 since we can then coerce other parts of xfs into
> > > shifting by ridiculously large values.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > v2: simplify ag min/max size definitions
> > > ---
> > >  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
> > >  fs/xfs/libxfs/xfs_sb.c |    3 +++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > index fc4386a..3ab1870 100644
> > > --- a/fs/xfs/libxfs/xfs_fs.h
> > > +++ b/fs/xfs/libxfs/xfs_fs.h
> > > @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
> > >  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
> > >  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
> > >  
> > > +/*
> > > + * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
> > > + * 16MB or larger than 1TB.
> > > + */
> > > +#define XFS_AG_MIN_BYTES	(1ULL << 24)	/* 16 MB */
> > > +#define XFS_AG_MAX_BYTES	(1ULL << 40)	/* 1 TB */
> > > +
> > 
> > Dave's comment aside... just a nit: the definitions above use
> > XFS_[MIN|MAX]_* naming rather than XFS_AG_*. It would be nice to be
> > consistent there if we stick with these.
> 
> Since I was extracting the constants from xfs_multidisk.h I thought it
> important to maintain the symbolic name to avoid breaking userspace.
> I guess we /could/ just add XFS_MIN_AG_BYTES to libxfs and change
> xfs_multidisk.h to do:
> 

Oh, I missed that..

> #define XFS_AG_MIN_BYTES	(XFS_MIN_AG_BYTES)
> 
> Or just decide that we don't care since we never install that header to
> /usr/include ?
> 

I think elevating these into libxfs warrant using whatever
sane/clean/consistent names fit in best from the perspective of libxfs.
If this is an internal define in xfsprogs, perhaps we can just
s/XFS_AG_MIN_BYTES/XFS_MIN_AG_BYTES/ there once this change trickles
down? Looks like it's only used in a handful of places.

Brian

> --D
> 
> > Brian
> > 
> > >  /* keep the maximum size under 2^31 by a small amount */
> > >  #define XFS_MAX_LOG_BYTES \
> > >  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > index 08e44a0..bdb4f74 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
> > >  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
> > >  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
> > >  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> > > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_AG_MIN_BYTES	||
> > > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_AG_MAX_BYTES	||
> > > +	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
> > >  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
> > >  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
> > >  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> > > --
> > > 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
> --
> 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 v2 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Darrick J. Wong]

On Tue, Jan 16, 2018 at 01:02:22PM -0500, Brian Foster wrote:
> On Tue, Jan 16, 2018 at 09:34:32AM -0800, Darrick J. Wong wrote:
> > On Tue, Jan 16, 2018 at 07:48:46AM -0500, Brian Foster wrote:
> > > On Mon, Jan 15, 2018 at 12:03:13PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Currently, we don't check sb_agblocks or sb_agblklog when we validate
> > > > the superblock, which means that we can fuzz garbage values into those
> > > > values and the mount succeeds.  This leads to all sorts of UBSAN
> > > > warnings in xfs/350 since we can then coerce other parts of xfs into
> > > > shifting by ridiculously large values.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > > v2: simplify ag min/max size definitions
> > > > ---
> > > >  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
> > > >  fs/xfs/libxfs/xfs_sb.c |    3 +++
> > > >  2 files changed, 10 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > > index fc4386a..3ab1870 100644
> > > > --- a/fs/xfs/libxfs/xfs_fs.h
> > > > +++ b/fs/xfs/libxfs/xfs_fs.h
> > > > @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
> > > >  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
> > > >  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
> > > >  
> > > > +/*
> > > > + * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
> > > > + * 16MB or larger than 1TB.
> > > > + */
> > > > +#define XFS_AG_MIN_BYTES	(1ULL << 24)	/* 16 MB */
> > > > +#define XFS_AG_MAX_BYTES	(1ULL << 40)	/* 1 TB */
> > > > +
> > > 
> > > Dave's comment aside... just a nit: the definitions above use
> > > XFS_[MIN|MAX]_* naming rather than XFS_AG_*. It would be nice to be
> > > consistent there if we stick with these.
> > 
> > Since I was extracting the constants from xfs_multidisk.h I thought it
> > important to maintain the symbolic name to avoid breaking userspace.
> > I guess we /could/ just add XFS_MIN_AG_BYTES to libxfs and change
> > xfs_multidisk.h to do:
> > 
> 
> Oh, I missed that..
> 
> > #define XFS_AG_MIN_BYTES	(XFS_MIN_AG_BYTES)
> > 
> > Or just decide that we don't care since we never install that header to
> > /usr/include ?
> > 
> 
> I think elevating these into libxfs warrant using whatever
> sane/clean/consistent names fit in best from the perspective of libxfs.
> If this is an internal define in xfsprogs, perhaps we can just
> s/XFS_AG_MIN_BYTES/XFS_MIN_AG_BYTES/ there once this change trickles
> down? Looks like it's only used in a handful of places.

Ok fair enough, will do.

--D

> Brian
> 
> > --D
> > 
> > > Brian
> > > 
> > > >  /* keep the maximum size under 2^31 by a small amount */
> > > >  #define XFS_MAX_LOG_BYTES \
> > > >  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > > index 08e44a0..bdb4f74 100644
> > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > @@ -253,6 +253,9 @@ xfs_mount_validate_sb(
> > > >  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
> > > >  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
> > > >  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> > > > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_AG_MIN_BYTES	||
> > > > +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_AG_MAX_BYTES	||
> > > > +	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
> > > >  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
> > > >  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
> > > >  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> > > > --
> > > > 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
> > --
> > 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

[PATCH v3 3/5] xfs: directory scrubber must walk through data block to offset

[Darrick J. Wong]

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

In xfs_scrub_dir_rec, we must walk through the directory block entries
to arrive at the offset given by the hash structure.  If we blindly
trust the hash address, we can end up midway into a directory entry and
stray outside the block.  Found by lastbit fuzzing lents[3].address in
xfs/390 with KASAN enabled.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v3: refactor endp users to call the helper
v2: improve defensive pointer checking (endp theoretically can be null)
---
 fs/xfs/libxfs/xfs_dir2.h      |    2 ++
 fs/xfs/libxfs/xfs_dir2_data.c |   43 +++++++++++++++++++++++------------------
 fs/xfs/libxfs/xfs_dir2_sf.c   |    4 +---
 fs/xfs/scrub/dir.c            |   38 +++++++++++++++++++++++++++++-------
 fs/xfs/xfs_dir2_readdir.c     |    4 +---
 5 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 1a8f2cf..388d67c 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -340,5 +340,7 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
 #define XFS_READDIR_BUFSIZE	(32768)
 
 unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
+void *xfs_dir3_data_endp(struct xfs_da_geometry *geo,
+		struct xfs_dir2_data_hdr *hdr);
 
 #endif	/* __XFS_DIR2_H__ */
diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 853d9ab..a1e30c7 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -89,7 +89,6 @@ __xfs_dir3_data_check(
 	case cpu_to_be32(XFS_DIR2_BLOCK_MAGIC):
 		btp = xfs_dir2_block_tail_p(geo, hdr);
 		lep = xfs_dir2_block_leaf_p(btp);
-		endp = (char *)lep;
 
 		/*
 		 * The number of leaf entries is limited by the size of the
@@ -104,11 +103,13 @@ __xfs_dir3_data_check(
 		break;
 	case cpu_to_be32(XFS_DIR3_DATA_MAGIC):
 	case cpu_to_be32(XFS_DIR2_DATA_MAGIC):
-		endp = (char *)hdr + geo->blksize;
 		break;
 	default:
 		return __this_address;
 	}
+	endp = xfs_dir3_data_endp(geo, hdr);
+	if (!endp)
+		return __this_address;
 
 	/*
 	 * Account for zero bestfree entries.
@@ -546,7 +547,6 @@ xfs_dir2_data_freescan_int(
 	struct xfs_dir2_data_hdr *hdr,
 	int			*loghead)
 {
-	xfs_dir2_block_tail_t	*btp;		/* block tail */
 	xfs_dir2_data_entry_t	*dep;		/* active data entry */
 	xfs_dir2_data_unused_t	*dup;		/* unused data entry */
 	struct xfs_dir2_data_free *bf;
@@ -568,12 +568,7 @@ xfs_dir2_data_freescan_int(
 	 * Set up pointers.
 	 */
 	p = (char *)ops->data_entry_p(hdr);
-	if (hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC) ||
-	    hdr->magic == cpu_to_be32(XFS_DIR3_BLOCK_MAGIC)) {
-		btp = xfs_dir2_block_tail_p(geo, hdr);
-		endp = (char *)xfs_dir2_block_leaf_p(btp);
-	} else
-		endp = (char *)hdr + geo->blksize;
+	endp = xfs_dir3_data_endp(geo, hdr);
 	/*
 	 * Loop over the block's entries.
 	 */
@@ -786,17 +781,9 @@ xfs_dir2_data_make_free(
 	/*
 	 * Figure out where the end of the data area is.
 	 */
-	if (hdr->magic == cpu_to_be32(XFS_DIR2_DATA_MAGIC) ||
-	    hdr->magic == cpu_to_be32(XFS_DIR3_DATA_MAGIC))
-		endptr = (char *)hdr + args->geo->blksize;
-	else {
-		xfs_dir2_block_tail_t	*btp;	/* block tail */
+	endptr = xfs_dir3_data_endp(args->geo, hdr);
+	ASSERT(endptr != NULL);
 
-		ASSERT(hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC) ||
-			hdr->magic == cpu_to_be32(XFS_DIR3_BLOCK_MAGIC));
-		btp = xfs_dir2_block_tail_p(args->geo, hdr);
-		endptr = (char *)xfs_dir2_block_leaf_p(btp);
-	}
 	/*
 	 * If this isn't the start of the block, then back up to
 	 * the previous entry and see if it's free.
@@ -1098,3 +1085,21 @@ xfs_dir2_data_use_free(
 	}
 	*needscanp = needscan;
 }
+
+/* Find the end of the entry data in a data/block format dir block. */
+void *
+xfs_dir3_data_endp(
+	struct xfs_da_geometry		*geo,
+	struct xfs_dir2_data_hdr	*hdr)
+{
+	switch (hdr->magic) {
+	case cpu_to_be32(XFS_DIR3_BLOCK_MAGIC):
+	case cpu_to_be32(XFS_DIR2_BLOCK_MAGIC):
+		return xfs_dir2_block_leaf_p(xfs_dir2_block_tail_p(geo, hdr));
+	case cpu_to_be32(XFS_DIR3_DATA_MAGIC):
+	case cpu_to_be32(XFS_DIR2_DATA_MAGIC):
+		return (char *)hdr + geo->blksize;
+	default:
+		return NULL;
+	}
+}
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 8500fa2..0c75a7f 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -156,7 +156,6 @@ xfs_dir2_block_to_sf(
 	xfs_dir2_sf_hdr_t	*sfhp)		/* shortform directory hdr */
 {
 	xfs_dir2_data_hdr_t	*hdr;		/* block header */
-	xfs_dir2_block_tail_t	*btp;		/* block tail pointer */
 	xfs_dir2_data_entry_t	*dep;		/* data entry pointer */
 	xfs_inode_t		*dp;		/* incore directory inode */
 	xfs_dir2_data_unused_t	*dup;		/* unused data pointer */
@@ -192,9 +191,8 @@ xfs_dir2_block_to_sf(
 	/*
 	 * Set up to loop over the block's entries.
 	 */
-	btp = xfs_dir2_block_tail_p(args->geo, hdr);
 	ptr = (char *)dp->d_ops->data_entry_p(hdr);
-	endptr = (char *)xfs_dir2_block_leaf_p(btp);
+	endptr = xfs_dir3_data_endp(args->geo, hdr);
 	sfep = xfs_dir2_sf_firstentry(sfp);
 	/*
 	 * Loop over the active and unused entries.
diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index f5a0d17..50b6a26 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -200,6 +200,7 @@ xfs_scrub_dir_rec(
 	struct xfs_inode		*dp = ds->dargs.dp;
 	struct xfs_dir2_data_entry	*dent;
 	struct xfs_buf			*bp;
+	char				*p, *endp;
 	xfs_ino_t			ino;
 	xfs_dablk_t			rec_bno;
 	xfs_dir2_db_t			db;
@@ -239,8 +240,35 @@ xfs_scrub_dir_rec(
 	}
 	xfs_scrub_buffer_recheck(ds->sc, bp);
 
-	/* Retrieve the entry, sanity check it, and compare hashes. */
 	dent = (struct xfs_dir2_data_entry *)(((char *)bp->b_addr) + off);
+
+	/* Make sure we got a real directory entry. */
+	p = (char *)mp->m_dir_inode_ops->data_entry_p(bp->b_addr);
+	endp = xfs_dir3_data_endp(mp->m_dir_geo, bp->b_addr);
+	if (!endp) {
+		xfs_scrub_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
+		goto out_relse;
+	}
+	while (p < endp) {
+		struct xfs_dir2_data_entry	*dep;
+		struct xfs_dir2_data_unused	*dup;
+
+		dup = (struct xfs_dir2_data_unused *)p;
+		if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
+			p += be16_to_cpu(dup->length);
+			continue;
+		}
+		dep = (struct xfs_dir2_data_entry *)p;
+		if (dep == dent)
+			break;
+		p += mp->m_dir_inode_ops->data_entsize(dep->namelen);
+	}
+	if (p >= endp) {
+		xfs_scrub_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
+		goto out_relse;
+	}
+
+	/* Retrieve the entry, sanity check it, and compare hashes. */
 	ino = be64_to_cpu(dent->inumber);
 	hash = be32_to_cpu(ent->hashval);
 	tag = be16_to_cpup(dp->d_ops->data_entry_tag_p(dent));
@@ -363,13 +391,7 @@ xfs_scrub_directory_data_bestfree(
 
 	/* Make sure the bestfrees are actually the best free spaces. */
 	ptr = (char *)d_ops->data_entry_p(bp->b_addr);
-	if (is_block) {
-		struct xfs_dir2_block_tail	*btp;
-
-		btp = xfs_dir2_block_tail_p(mp->m_dir_geo, bp->b_addr);
-		endptr = (char *)xfs_dir2_block_leaf_p(btp);
-	} else
-		endptr = (char *)bp->b_addr + BBTOB(bp->b_length);
+	endptr = xfs_dir3_data_endp(mp->m_dir_geo, bp->b_addr);
 
 	/* Iterate the entries, stopping when we hit or go past the end. */
 	while (ptr < endptr) {
diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
index 0c58918..b6ae359 100644
--- a/fs/xfs/xfs_dir2_readdir.c
+++ b/fs/xfs/xfs_dir2_readdir.c
@@ -152,7 +152,6 @@ xfs_dir2_block_getdents(
 	struct xfs_inode	*dp = args->dp;	/* incore directory inode */
 	xfs_dir2_data_hdr_t	*hdr;		/* block header */
 	struct xfs_buf		*bp;		/* buffer for block */
-	xfs_dir2_block_tail_t	*btp;		/* block tail */
 	xfs_dir2_data_entry_t	*dep;		/* block data entry */
 	xfs_dir2_data_unused_t	*dup;		/* block unused entry */
 	char			*endptr;	/* end of the data entries */
@@ -185,9 +184,8 @@ xfs_dir2_block_getdents(
 	/*
 	 * Set up values for the loop.
 	 */
-	btp = xfs_dir2_block_tail_p(geo, hdr);
 	ptr = (char *)dp->d_ops->data_entry_p(hdr);
-	endptr = (char *)xfs_dir2_block_leaf_p(btp);
+	endptr = xfs_dir3_data_endp(geo, hdr);
 
 	/*
 	 * Loop over the data portion of the block.

Re: [PATCH v2 4/5] xfs: attr leaf verifier needs to check for obviously bad count

[Darrick J. Wong]

On Tue, Jan 16, 2018 at 07:50:45AM -0500, Brian Foster wrote:
> On Mon, Jan 15, 2018 at 12:05:27PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In the attribute leaf verifier, we can check for obviously bad values of
> > firstused and count so that later attempts at lasthash don't run off the
> > end of the memory buffer.  Found by ones fuzzing hdr.count in xfs/400 with
> > KASAN.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: strengthen checking, clarify what we're testing via comments
> > ---
> 
> Ok, digging more into the code I see that firstused refers to the lowest
> ->nameidx of the xfs_attr_leaf_entry's in the block. On insert,
> ->nameidx is constructed from freemap.base, which starts at just beyond
> the block header. freemap.base+freemap.size essentially refers to a raw
> block offset where the name/value info is placed, so I think I just
> misread that code the first time around. This makes sense and the
> cleanups look good. Thanks for the clarification.
> 
> One more small question..
> 
> >  fs/xfs/libxfs/xfs_attr_leaf.c |   26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index 6fddce7..f1a1c60 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk(
> >  
> >  static xfs_failaddr_t
> >  xfs_attr3_leaf_verify(
> > -	struct xfs_buf		*bp)
> > +	struct xfs_buf			*bp)
> >  {
> > -	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > -	struct xfs_attr_leafblock *leaf = bp->b_addr;
> > -	struct xfs_perag *pag = bp->b_pag;
> > -	struct xfs_attr3_icleaf_hdr ichdr;
> > +	struct xfs_attr3_icleaf_hdr	ichdr;
> > +	struct xfs_mount		*mp = bp->b_target->bt_mount;
> > +	struct xfs_attr_leafblock	*leaf = bp->b_addr;
> > +	struct xfs_perag		*pag = bp->b_pag;
> > +	struct xfs_attr_leaf_entry	*entries;
> >  
> >  	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
> >  
> > @@ -282,6 +283,21 @@ xfs_attr3_leaf_verify(
> >  	if (pag && pag->pagf_init && ichdr.count == 0)
> >  		return __this_address;
> >  
> > +	/*
> > +	 * firstused is the block offset of the first name info structure.
> > +	 * Make sure it doesn't go off the block or crash into the header.
> > +	 */
> > +	if (ichdr.firstused > mp->m_attr_geo->blksize)
> > +		return __this_address;
> > +	if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf))
> > +		return __this_address;
> > +
> > +	/* Make sure the entries array doesn't crash into the name info. */
> > +	entries = xfs_attr3_leaf_entryp(bp->b_addr);
> > +	if ((char *)&entries[ichdr.count] >=
> > +	    (char *)bp->b_addr + ichdr.firstused)
> > +		return __this_address;
> > +
> 
> Can the end of the entries list align with the first namelist object if
> the block is full, for example? E.g., is '&entries[ichdr.count] ==
> bp->b_addr + ichdr.firstused' a sane possibility?

Yes.  Good catch!  I'll change the '>=' to '>'.

--D

> That aside, this looks good to me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  	/* XXX: need to range check rest of attr header values */
> >  	/* XXX: hash order check? */
> >  
> > --
> > 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

Re: [PATCH v3 3/5] xfs: directory scrubber must walk through data block to offset

[Dave Chinner]

On Tue, Jan 16, 2018 at 03:30:45PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_scrub_dir_rec, we must walk through the directory block entries
> to arrive at the offset given by the hash structure.  If we blindly
> trust the hash address, we can end up midway into a directory entry and
> stray outside the block.  Found by lastbit fuzzing lents[3].address in
> xfs/390 with KASAN enabled.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: refactor endp users to call the helper
> v2: improve defensive pointer checking (endp theoretically can be null)

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

[PATCH v3 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Darrick J. Wong]

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

Currently, we don't check sb_agblocks or sb_agblklog when we validate
the superblock, which means that we can fuzz garbage values into those
values and the mount succeeds.  This leads to all sorts of UBSAN
warnings in xfs/350 since we can then coerce other parts of xfs into
shifting by ridiculously large values.

Once we've validated agblocks, make sure the agcount makes sense.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v3: also check that dblocks/agcount/agblocks make sense
v2: simplify ag min/max size definitions
---
 fs/xfs/libxfs/xfs_fs.h |    7 +++++++
 fs/xfs/libxfs/xfs_sb.c |   14 ++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index fc4386a..1de8555 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
 #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
 #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
 
+/*
+ * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
+ * 16MB or larger than 1TB.
+ */
+#define XFS_MIN_AG_BYTES	(1ULL << 24)	/* 16 MB */
+#define XFS_MAX_AG_BYTES	(1ULL << 40)	/* 1 TB */
+
 /* keep the maximum size under 2^31 by a small amount */
 #define XFS_MAX_LOG_BYTES \
 	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 08e44a0..e2b82d7 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -119,6 +119,9 @@ xfs_mount_validate_sb(
 	bool		check_inprogress,
 	bool		check_version)
 {
+	u32		agcount = 0;
+	u32		rem;
+
 	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
 		xfs_warn(mp, "bad magic number");
 		return -EWRONGFS;
@@ -229,6 +232,13 @@ xfs_mount_validate_sb(
 		return -EINVAL;
 	}
 
+	/* Compute agcount for this number of dblocks and agblocks */
+	if (sbp->sb_agblocks) {
+		agcount = div_u64_rem(sbp->sb_dblocks, sbp->sb_agblocks, &rem);
+		if (rem)
+			agcount++;
+	}
+
 	/*
 	 * More sanity checking.  Most of these were stolen directly from
 	 * xfs_repair.
@@ -253,6 +263,10 @@ xfs_mount_validate_sb(
 	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
 	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
 	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
+	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_MIN_AG_BYTES	||
+	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_MAX_AG_BYTES	||
+	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
+	    agcount == 0 || agcount != sbp->sb_agcount			||
 	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||

Re: [PATCH v3 1/5] xfs: check sb_agblocks and sb_agblklog when validating superblock

[Brian Foster]

On Tue, Jan 16, 2018 at 05:20:22PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Currently, we don't check sb_agblocks or sb_agblklog when we validate
> the superblock, which means that we can fuzz garbage values into those
> values and the mount succeeds.  This leads to all sorts of UBSAN
> warnings in xfs/350 since we can then coerce other parts of xfs into
> shifting by ridiculously large values.
> 
> Once we've validated agblocks, make sure the agcount makes sense.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: also check that dblocks/agcount/agblocks make sense
> v2: simplify ag min/max size definitions
> ---

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

>  fs/xfs/libxfs/xfs_fs.h |    7 +++++++
>  fs/xfs/libxfs/xfs_sb.c |   14 ++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index fc4386a..1de8555 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -233,6 +233,13 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_MAX_LOG_BLOCKS	(1024 * 1024ULL)
>  #define XFS_MIN_LOG_BYTES	(10 * 1024 * 1024ULL)
>  
> +/*
> + * Limits on sb_agblocks/sb_agblklog -- mkfs won't format AGs smaller than
> + * 16MB or larger than 1TB.
> + */
> +#define XFS_MIN_AG_BYTES	(1ULL << 24)	/* 16 MB */
> +#define XFS_MAX_AG_BYTES	(1ULL << 40)	/* 1 TB */
> +
>  /* keep the maximum size under 2^31 by a small amount */
>  #define XFS_MAX_LOG_BYTES \
>  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 08e44a0..e2b82d7 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -119,6 +119,9 @@ xfs_mount_validate_sb(
>  	bool		check_inprogress,
>  	bool		check_version)
>  {
> +	u32		agcount = 0;
> +	u32		rem;
> +
>  	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
>  		xfs_warn(mp, "bad magic number");
>  		return -EWRONGFS;
> @@ -229,6 +232,13 @@ xfs_mount_validate_sb(
>  		return -EINVAL;
>  	}
>  
> +	/* Compute agcount for this number of dblocks and agblocks */
> +	if (sbp->sb_agblocks) {
> +		agcount = div_u64_rem(sbp->sb_dblocks, sbp->sb_agblocks, &rem);
> +		if (rem)
> +			agcount++;
> +	}
> +
>  	/*
>  	 * More sanity checking.  Most of these were stolen directly from
>  	 * xfs_repair.
> @@ -253,6 +263,10 @@ xfs_mount_validate_sb(
>  	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
>  	    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE			||
>  	    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
> +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) < XFS_MIN_AG_BYTES	||
> +	    XFS_FSB_TO_B(mp, sbp->sb_agblocks) > XFS_MAX_AG_BYTES	||
> +	    sbp->sb_agblklog != xfs_highbit32(sbp->sb_agblocks - 1) + 1	||
> +	    agcount == 0 || agcount != sbp->sb_agcount			||
>  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> --
> 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