diff mbox series

[4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage

Message ID 160375513815.879169.8550751453198927018.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfsprogs: fixes for 5.10 | expand

Commit Message

Darrick J. Wong Oct. 26, 2020, 11:32 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

In validate_ag[fi], we should check that the levels of the rmap and
refcount btrees are valid.  If they aren't, we need to tell phase4 to
skip the comparison between the existing and incore rmap and refcount
data.  The comparison routines use libxfs btree cursors, which assume
that the caller validated bc_nlevels and will corrupt memory if we load
a btree cursor with a garbage level count.

This was found by examing a core dump from a failed xfs/086 invocation.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |   36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

Comments

Allison Henderson Oct. 27, 2020, 5:35 a.m. UTC | #1 
On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In validate_ag[fi], we should check that the levels of the rmap and
> refcount btrees are valid.  If they aren't, we need to tell phase4 to
> skip the comparison between the existing and incore rmap and refcount
> data.  The comparison routines use libxfs btree cursors, which assume
> that the caller validated bc_nlevels and will corrupt memory if we load
> a btree cursor with a garbage level count.
> 
> This was found by examing a core dump from a failed xfs/086 invocation.
> 
Ok, looks ok
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   repair/scan.c |   36 ++++++++++++++++++++++++++----------
>   1 file changed, 26 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/repair/scan.c b/repair/scan.c
> index 42b299f75067..2a38ae5197c6 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -2279,23 +2279,31 @@ validate_agf(
>   
>   	if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
>   		struct rmap_priv	priv;
> +		unsigned int		levels;
>   
>   		memset(&priv.high_key, 0xFF, sizeof(priv.high_key));
>   		priv.high_key.rm_blockcount = 0;
>   		priv.agcnts = agcnts;
>   		priv.last_rec.rm_owner = XFS_RMAP_OWN_UNKNOWN;
>   		priv.nr_blocks = 0;
> +
> +		levels = be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]);
> +		if (levels >= XFS_BTREE_MAXLEVELS) {
> +			do_warn(_("bad levels %u for rmapbt root, agno %d\n"),
> +				levels, agno);
> +			rmap_avoid_check();
> +		}
> +
>   		bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
>   		if (libxfs_verify_agbno(mp, agno, bno)) {
> -			scan_sbtree(bno,
> -				    be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]),
> -				    agno, 0, scan_rmapbt, 1, XFS_RMAP_CRC_MAGIC,
> -				    &priv, &xfs_rmapbt_buf_ops);
> +			scan_sbtree(bno, levels, agno, 0, scan_rmapbt, 1,
> +					XFS_RMAP_CRC_MAGIC, &priv,
> +					&xfs_rmapbt_buf_ops);
>   			if (be32_to_cpu(agf->agf_rmap_blocks) != priv.nr_blocks)
>   				do_warn(_("bad rmapbt block count %u, saw %u\n"),
>   					priv.nr_blocks,
>   					be32_to_cpu(agf->agf_rmap_blocks));
> -		} else  {
> +		} else {
>   			do_warn(_("bad agbno %u for rmapbt root, agno %d\n"),
>   				bno, agno);
>   			rmap_avoid_check();
> @@ -2303,20 +2311,28 @@ validate_agf(
>   	}
>   
>   	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> +		unsigned int	levels;
> +
> +		levels = be32_to_cpu(agf->agf_refcount_level);
> +		if (levels >= XFS_BTREE_MAXLEVELS) {
> +			do_warn(_("bad levels %u for refcountbt root, agno %d\n"),
> +				levels, agno);
> +			refcount_avoid_check();
> +		}
> +
>   		bno = be32_to_cpu(agf->agf_refcount_root);
>   		if (libxfs_verify_agbno(mp, agno, bno)) {
>   			struct refc_priv	priv;
>   
>   			memset(&priv, 0, sizeof(priv));
> -			scan_sbtree(bno,
> -				    be32_to_cpu(agf->agf_refcount_level),
> -				    agno, 0, scan_refcbt, 1, XFS_REFC_CRC_MAGIC,
> -				    &priv, &xfs_refcountbt_buf_ops);
> +			scan_sbtree(bno, levels, agno, 0, scan_refcbt, 1,
> +					XFS_REFC_CRC_MAGIC, &priv,
> +					&xfs_refcountbt_buf_ops);
>   			if (be32_to_cpu(agf->agf_refcount_blocks) != priv.nr_blocks)
>   				do_warn(_("bad refcountbt block count %u, saw %u\n"),
>   					priv.nr_blocks,
>   					be32_to_cpu(agf->agf_refcount_blocks));
> -		} else  {
> +		} else {
>   			do_warn(_("bad agbno %u for refcntbt root, agno %d\n"),
>   				bno, agno);
>   			refcount_avoid_check();
>
Christoph Hellwig Oct. 28, 2020, 7:34 a.m. UTC | #2 
On Mon, Oct 26, 2020 at 04:32:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In validate_ag[fi], we should check that the levels of the rmap and
> refcount btrees are valid.  If they aren't, we need to tell phase4 to
> skip the comparison between the existing and incore rmap and refcount
> data.  The comparison routines use libxfs btree cursors, which assume
> that the caller validated bc_nlevels and will corrupt memory if we load
> a btree cursor with a garbage level count.
> 
> This was found by examing a core dump from a failed xfs/086 invocation.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/repair/scan.c b/repair/scan.c
index 42b299f75067..2a38ae5197c6 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -2279,23 +2279,31 @@  validate_agf(
 
 	if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
 		struct rmap_priv	priv;
+		unsigned int		levels;
 
 		memset(&priv.high_key, 0xFF, sizeof(priv.high_key));
 		priv.high_key.rm_blockcount = 0;
 		priv.agcnts = agcnts;
 		priv.last_rec.rm_owner = XFS_RMAP_OWN_UNKNOWN;
 		priv.nr_blocks = 0;
+
+		levels = be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]);
+		if (levels >= XFS_BTREE_MAXLEVELS) {
+			do_warn(_("bad levels %u for rmapbt root, agno %d\n"),
+				levels, agno);
+			rmap_avoid_check();
+		}
+
 		bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
 		if (libxfs_verify_agbno(mp, agno, bno)) {
-			scan_sbtree(bno,
-				    be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]),
-				    agno, 0, scan_rmapbt, 1, XFS_RMAP_CRC_MAGIC,
-				    &priv, &xfs_rmapbt_buf_ops);
+			scan_sbtree(bno, levels, agno, 0, scan_rmapbt, 1,
+					XFS_RMAP_CRC_MAGIC, &priv,
+					&xfs_rmapbt_buf_ops);
 			if (be32_to_cpu(agf->agf_rmap_blocks) != priv.nr_blocks)
 				do_warn(_("bad rmapbt block count %u, saw %u\n"),
 					priv.nr_blocks,
 					be32_to_cpu(agf->agf_rmap_blocks));
-		} else  {
+		} else {
 			do_warn(_("bad agbno %u for rmapbt root, agno %d\n"),
 				bno, agno);
 			rmap_avoid_check();
@@ -2303,20 +2311,28 @@  validate_agf(
 	}
 
 	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
+		unsigned int	levels;
+
+		levels = be32_to_cpu(agf->agf_refcount_level);
+		if (levels >= XFS_BTREE_MAXLEVELS) {
+			do_warn(_("bad levels %u for refcountbt root, agno %d\n"),
+				levels, agno);
+			refcount_avoid_check();
+		}
+
 		bno = be32_to_cpu(agf->agf_refcount_root);
 		if (libxfs_verify_agbno(mp, agno, bno)) {
 			struct refc_priv	priv;
 
 			memset(&priv, 0, sizeof(priv));
-			scan_sbtree(bno,
-				    be32_to_cpu(agf->agf_refcount_level),
-				    agno, 0, scan_refcbt, 1, XFS_REFC_CRC_MAGIC,
-				    &priv, &xfs_refcountbt_buf_ops);
+			scan_sbtree(bno, levels, agno, 0, scan_refcbt, 1,
+					XFS_REFC_CRC_MAGIC, &priv,
+					&xfs_refcountbt_buf_ops);
 			if (be32_to_cpu(agf->agf_refcount_blocks) != priv.nr_blocks)
 				do_warn(_("bad refcountbt block count %u, saw %u\n"),
 					priv.nr_blocks,
 					be32_to_cpu(agf->agf_refcount_blocks));
-		} else  {
+		} else {
 			do_warn(_("bad agbno %u for refcntbt root, agno %d\n"),
 				bno, agno);
 			refcount_avoid_check();