Try to squash metadump data leaks

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

db/metadump.c +23 -4

index cc2ae9af..97d2a490 100644
@@ -1426,7 +1426,7 @@ process_dir_leaf_block(
 	char				*block)
 {
 	struct xfs_dir2_leaf		*leaf;
-	struct xfs_dir3_icleaf_hdr 	leafhdr;
+	struct xfs_dir3_icleaf_hdr	leafhdr;
 
 	if (!zero_stale_data)
 		return;

@@ -1435,20 +1435,39 @@ process_dir_leaf_block(
 	leaf = (struct xfs_dir2_leaf *)block;
 	M_DIROPS(mp)->leaf_hdr_from_disk(&leafhdr, leaf);
 
-	/* Zero out space from end of ents[] to bests */
-	if (leafhdr.magic == XFS_DIR2_LEAF1_MAGIC ||
-	    leafhdr.magic == XFS_DIR3_LEAF1_MAGIC) {
+	switch (leafhdr.magic) {
+	case XFS_DIR2_LEAF1_MAGIC:
+	case XFS_DIR3_LEAF1_MAGIC: {
 		struct xfs_dir2_leaf_tail	*ltp;
 		__be16				*lbp;
 		struct xfs_dir2_leaf_entry	*ents;
 		char				*free; /* end of ents */
 
+		/* Zero out space from end of ents[] to bests */
 		ents = M_DIROPS(mp)->leaf_ents_p(leaf);
 		free = (char *)&ents[leafhdr.count];
 		ltp = xfs_dir2_leaf_tail_p(mp->m_dir_geo, leaf);
 		lbp = xfs_dir2_leaf_bests_p(ltp);
 		memset(free, 0, (char *)lbp - free);
 		iocur_top->need_crc = 1;
+		break;
+	}
+	case XFS_DIR2_LEAFN_MAGIC:
+	case XFS_DIR3_LEAFN_MAGIC: {
+		struct xfs_dir2_leaf_entry	*ents;
+		char				*free;
+		int				used;
+
+		/* Zero out space from end of ents[] to end of block */
+		ents = M_DIROPS(mp)->leaf_ents_p(leaf);
+		free = (char *)&ents[leafhdr.count];
+		used = free - (char*)leaf;
+		memset(free, 0, mp->m_dir_geo->blksize - used);
+		iocur_top->need_crc = 1;
+		break;
+	}
+	default:
+		break;
 	}
 }
 

Using basically the same code as in process_single_fsb_objects.

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

db/metadump.c +1 -0

index 97d2a490..be7cf360 100644
@@ -1881,6 +1881,7 @@ process_multi_fsb_objects(
 	typnm_t		btype,
 	xfs_fileoff_t	last)
 {
+	char		*dp;
 	int		ret = 0;
 
 	switch (btype) {

db/metadump.c +11 -6

-			     o >= mp->m_dir_geo->leafblk) {
-				ret = write_buf(iocur_top);
-				goto out_pop;
+			dp = iocur_top->data;
+			if (o >= mp->m_dir_geo->freeblk) {
+				/* TODO, zap any stale data */
+				goto write;
+			} else if (o >= mp->m_dir_geo->leafblk) {
+				process_dir_leaf_block(dp);
+			} else {
+				process_dir_data_block(dp, o,
+					 last == mp->m_dir_geo->fsbcount);
 			}
 
-			process_dir_data_block(iocur_top->data, o,
-					       last == mp->m_dir_geo->fsbcount);
-			iocur_top->need_crc = 1;
+			iocur_top->need_crc = obfuscate || zero_stale_data;
+write:
 			ret = write_buf(iocur_top);
 out_pop:
 			pop_cur();

Signature · 1 line

2.14.5

Quoted 13 lines · Stefan Ring

Using basically the same code as in process_single_fsb_objects.

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

db/metadump.c +0 -0

index 97d2a490..be7cf360 100644
@@ -1921,15 +1922,19 @@ process_multi_fsb_objects(
 

db/metadump.c process_multi_fsb_objects( · line 1924

Quoted 3 lines

db/metadump.c +0 -1

                      }
 
-                     if ((!obfuscate && !zero_stale_data) ||

Hmmm, if we're not obfuscating or zeroing data, can we still jump to write_buf()?

--D

Quoted 26 lines

db/metadump.c +11 -7

-                          o >= mp->m_dir_geo->leafblk) {
-                             ret = write_buf(iocur_top);
-                             goto out_pop;
+                     dp = iocur_top->data;
+                     if (o >= mp->m_dir_geo->freeblk) {
+                             /* TODO, zap any stale data */
+                             goto write;
+                     } else if (o >= mp->m_dir_geo->leafblk) {
+                             process_dir_leaf_block(dp);
+                     } else {
+                             process_dir_data_block(dp, o,
+                                      last == mp->m_dir_geo->fsbcount);
                      }
 
-                     process_dir_data_block(iocur_top->data, o,
-                                            last == mp->m_dir_geo->fsbcount);
-                     iocur_top->need_crc = 1;
+                     iocur_top->need_crc = obfuscate || zero_stale_data;
+write:
                      ret = write_buf(iocur_top);
 out_pop:
                      pop_cur();
--

2.14.5

Quoted 21 lines · Darrick J. Wong

Quoted 12 lines · Stefan Ring

Using basically the same code as in process_single_fsb_objects.

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

db/metadump.c +0 -0

index 97d2a490..be7cf360 100644

db/metadump.c process_multi_fsb_objects( · line 1924

Quoted 4 lines

db/metadump.c +0 -1

@@ -1921,15 +1922,19 @@ process_multi_fsb_objects(
 
                      }
 
-                     if ((!obfuscate && !zero_stale_data) ||

Hmmm, if we're not obfuscating or zeroing data, can we still jump to write_buf()?

Quoted 1 line

--D

The call to write_buf() is always necessary, as without this, nothing would end up in the metadump. But while trying to argue the soundness of this patch, I have found deficiencies ;). It will always call process_dir_data_block, even without any zeroing/obfuscation flags set.

I must have considered this harmless, since it does not actually "do" anything, but I'm not so sure anymore because it may complain about data inconsistencies, and this might make an otherwise dumpable image non-dumpable.

It is also, at least in theory, able to reset the need_crc back to zero, contrary to all the other usages of this flag in this file. I don't think this is an actual problem, as the flag only seems to be set when zeroing or obfuscating in the first place, but it might become one if circumstances far removed from this location change. It is just too much of a mental burden.

I will redo this once again so that it will be more similar in structure to the code in process_single_fsb_objects _and_ the original code, I hope.

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

db/metadump.c +40 -6

index be7cf360..d4c751c0 100644
@@ -1421,6 +1421,43 @@ process_sf_attr(
 		memset(asfep, 0, XFS_DFORK_ASIZE(dip, mp) - ino_attr_size);
 }
 
+static void
+process_dir_free_block(
+	char				*block)
+{
+	struct xfs_dir2_free		*free;
+	struct xfs_dir3_icfree_hdr	freehdr;
+
+	if (!zero_stale_data)
+		return;
+
+	free = (struct xfs_dir2_free *)block;
+	M_DIROPS(mp)->free_hdr_from_disk(&freehdr, free);
+
+	switch (freehdr.magic) {
+	case XFS_DIR2_FREE_MAGIC:
+	case XFS_DIR3_FREE_MAGIC: {
+		__be16			*bests;
+		char			*high;
+		int			used;
+
+		/* Zero out space from end of bests[] to end of block */
+		bests = M_DIROPS(mp)->free_bests_p(free);
+		high = (char *)&bests[freehdr.nvalid];
+		used = high - (char*)free;
+		memset(high, 0, mp->m_dir_geo->blksize - used);
+		iocur_top->need_crc = 1;
+		break;
+	}
+	default:
+		if (show_warnings)
+			print_warning("invalid magic in dir inode %llu "
+				      "free block",
+				      (unsigned long long)cur_ino);
+		break;
+	}
+}
+
 static void
 process_dir_leaf_block(
 	char				*block)

@@ -1518,7 +1555,7 @@ process_dir_data_block(
 		if (show_warnings)
 			print_warning(
 		"invalid magic in dir inode %llu block %ld",
-					(long long)cur_ino, (long)offset);
+					(unsigned long long)cur_ino, (long)offset);
 		return;
 	}
 

@@ -1832,8 +1869,7 @@ process_single_fsb_objects(
 		switch (btype) {
 		case TYP_DIR2:
 			if (o >= mp->m_dir_geo->freeblk) {
-				/* TODO, zap any stale data */
-				break;
+				process_dir_free_block(dp);
 			} else if (o >= mp->m_dir_geo->leafblk) {
 				process_dir_leaf_block(dp);
 			} else {

@@ -1924,8 +1960,7 @@ process_multi_fsb_objects(
 
 			dp = iocur_top->data;
 			if (o >= mp->m_dir_geo->freeblk) {
-				/* TODO, zap any stale data */
-				goto write;
+				process_dir_free_block(dp);
 			} else if (o >= mp->m_dir_geo->leafblk) {
 				process_dir_leaf_block(dp);
 			} else {

@@ -1934,7 +1969,6 @@ process_multi_fsb_objects(
 			}
 
 			iocur_top->need_crc = obfuscate || zero_stale_data;
-write:
 			ret = write_buf(iocur_top);
 out_pop:
 			pop_cur();

Signature · 1 line

2.14.5

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

db/metadump.c +13 -0

index d4c751c0..59765263 100644
@@ -2173,6 +2173,19 @@ process_btinode(
 	}
 
 	pp = XFS_BMDR_PTR_ADDR(dib, 1, maxrecs);
+
+	if (zero_stale_data) {
+		char	*top;
+
+		/* Unused btree key space */
+		top = (char*)XFS_BMDR_KEY_ADDR(dib, nrecs + 1);
+		memset(top, 0, (char*)pp - top);
+
+		/* Unused btree ptr space */
+		top = (char*)&pp[nrecs];
+		memset(top, 0, (char*)dib + XFS_DFORK_SIZE(dip, mp, whichfork) - top);
+	}
+
 	for (i = 0; i < nrecs; i++) {
 		xfs_agnumber_t	ag;
 		xfs_agblock_t	bno;

Signature · 1 line

2.14.5

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

db/metadump.c +3 -0

index 59765263..57d6bc09 100644
@@ -2269,6 +2269,25 @@ process_inode_data(
 	return 1;
 }
 
+static int
+process_dev_inode(
+	xfs_dinode_t		*dip)

db/metadump.c +15 -1

+					XFS_DFORK_DSIZE(dip, mp) - size);
+		}
+		return 1;
+	}
+}
+
 /*
  * when we process the inode, we may change the data in the data and/or
  * attribute fork if they are in short form and we are obfuscating names.

@@ -2321,7 +2340,15 @@ process_inode(
 		case S_IFREG:
 			success = process_inode_data(dip, TYP_DATA);
 			break;
-		default: ;
+		case S_IFIFO:
+		case S_IFCHR:
+		case S_IFBLK:
+		case S_IFSOCK:
+			success = process_dev_inode(dip);
+			need_new_crc = 1;
+			break;
+		default:
+			break;
 	}
 	nametable_clear();
 

Signature · 1 line

2.14.5

Another round... I'm starting to feel quite confident about it this time.

Since last time:

- Fixed the attribute corruption in the last patch.
- Changed the accidental 'a' and 'b' I had left in the code back to zeroes.
- Brought the flag for crc recalc a tiny bit more in line with the single_fsb code.
- Fixed the wrong control flow in the interim patch which duplicates a TODO comment in the code only to get rid of it in the next patch.

Stefan Ring (5):
xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks
xfs_metadump: Zap multi fsb blocks
xfs_metadump: Zap freeindex blocks in directory inodes
xfs_metadump: Zap unused space in inode btrees
xfs_metadump: Zap dev inodes

 db/metadump.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 113 insertions(+), 15 deletions(-)

Quoted 18 lines · Stefan Ring

Another round... I'm starting to feel quite confident about it this time.

Since last time:

- Fixed the attribute corruption in the last patch.
- Changed the accidental 'a' and 'b' I had left in the code back to zeroes.
- Brought the flag for crc recalc a tiny bit more in line with the single_fsb code.
- Fixed the wrong control flow in the interim patch which duplicates a TODO comment in the code only to get rid of it in the next patch.

Stefan Ring (5):
xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks
xfs_metadump: Zap multi fsb blocks
xfs_metadump: Zap freeindex blocks in directory inodes
xfs_metadump: Zap unused space in inode btrees
xfs_metadump: Zap dev inodes

Quoted 2 lines

 db/metadump.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 113 insertions(+), 15 deletions(-)

Is this an appropriate time interval for pinging?

Using basically the same code as in process_single_fsb_objects.

Signed-off-by Stefan Ring <[email protected]>

---
 db/metadump.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

db/metadump.c +14 -7

index 97d2a490..45705685 100644
@@ -1881,6 +1881,7 @@ process_multi_fsb_objects(
 	typnm_t		btype,
 	xfs_fileoff_t	last)
 {
+	char		*dp;
 	int		ret = 0;
 
 	switch (btype) {

@@ -1921,15 +1922,21 @@ process_multi_fsb_objects(
 
 			}
 
-			if ((!obfuscate && !zero_stale_data) ||
-			     o >= mp->m_dir_geo->leafblk) {
-				ret = write_buf(iocur_top);
-				goto out_pop;
-			}
+			if (!obfuscate && !zero_stale_data)
+				goto write;
 
-			process_dir_data_block(iocur_top->data, o,
-					       last == mp->m_dir_geo->fsbcount);
+			dp = iocur_top->data;
+			if (o >= mp->m_dir_geo->freeblk) {
+				/* TODO, zap any stale data */
+				goto write;
+			} else if (o >= mp->m_dir_geo->leafblk) {
+				process_dir_leaf_block(dp);
+			} else {
+				process_dir_data_block(dp, o,
+					 last == mp->m_dir_geo->fsbcount);
+			}
 			iocur_top->need_crc = 1;
+write:
 			ret = write_buf(iocur_top);
 out_pop:
 			pop_cur();

Quoted 3 lines · Stefan Ring

Using basically the same code as in process_single_fsb_objects.

Signed-off-by Stefan Ring <[email protected]>

thanks, the change here to goto write: looks good to me.

Reviewed-by Eric Sandeen <[email protected]>

Quoted 6 lines · Eric Sandeen

Quoted 3 lines · Stefan Ring

Using basically the same code as in process_single_fsb_objects.

Signed-off-by Stefan Ring <[email protected]>

Quoted 3 lines

thanks, the change here to goto write: looks good to me.

Reviewed-by Eric Sandeen <[email protected]>

Thanks, this makes my series fully reviewed. Is there anything left for me to do now? FWIW, I added the Reviewed-by tag and pushed it to
<https://github.com/Ringdingcoder/xfsprogs-dev/commits/metadump-fully-reviewed>.