[PATCH v5 0/5] Try to squash metadump data leaks

[PATCH v5 0/5] Try to squash metadump data leaks

[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

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

-- 
2.14.5

[PATCH 1/5] xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
---
 db/metadump.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index cc2ae9af..97d2a490 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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;
 	}
 }
 
-- 
2.14.5

Re: [PATCH 1/5] xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks

[Eric Sandeen]

On 11/5/18 3:31 PM, Stefan Ring wrote:
> Signed-off-by: Stefan Ring <stefanrin@gmail.com>

Sorry this took so long.  I didn't forget about you!
And I really appreciate the work.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  db/metadump.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index cc2ae9af..97d2a490 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -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;
>  	}
>  }
>  
> 

[PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Stefan Ring]

Using basically the same code as in process_single_fsb_objects.

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
---
 db/metadump.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 97d2a490..be7cf360 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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,19 @@ process_multi_fsb_objects(
 
 			}
 
-			if ((!obfuscate && !zero_stale_data) ||
-			     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

Re: [PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Darrick J. Wong]

On Mon, Nov 05, 2018 at 10:31:42PM +0100, Stefan Ring wrote:
> Using basically the same code as in process_single_fsb_objects.
> 
> Signed-off-by: Stefan Ring <stefanrin@gmail.com>
> ---
>  db/metadump.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 97d2a490..be7cf360 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -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,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()?

--D

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

Re: [PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Stefan Ring]

On Thu, Jan 3, 2019 at 6:55 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Mon, Nov 05, 2018 at 10:31:42PM +0100, Stefan Ring wrote:
> > Using basically the same code as in process_single_fsb_objects.
> >
> > Signed-off-by: Stefan Ring <stefanrin@gmail.com>
> > ---
> >  db/metadump.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/db/metadump.c b/db/metadump.c
> > index 97d2a490..be7cf360 100644
> > --- a/db/metadump.c
> > +++ b/db/metadump.c
> > @@ -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()?
>
> --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.

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

Re: [PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Eric Sandeen]

On 1/4/19 4:27 PM, Stefan Ring wrote:
> On Thu, Jan 3, 2019 at 6:55 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>> On Mon, Nov 05, 2018 at 10:31:42PM +0100, Stefan Ring wrote:
>>> Using basically the same code as in process_single_fsb_objects.
>>>
>>> Signed-off-by: Stefan Ring <stefanrin@gmail.com>
>>> ---
>>>  db/metadump.c | 19 ++++++++++++-------
>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/db/metadump.c b/db/metadump.c
>>> index 97d2a490..be7cf360 100644
>>> --- a/db/metadump.c
>>> +++ b/db/metadump.c
>>> @@ -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()?
>>
>> --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.

Right, I think that was Darrick's point.  (I had noticed this too but
have been woefully slow in replying.)

> 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.

Yup, exactly.  You're good!  ;)

> 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 feel like the whole need_crc business could be cleaned up, TBH (later).

> 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.

FWIW, I've been running all of xfstests in various permutations with a patch
to dump and restore and xfs_repair an image after each run, using each of
"" "-o" "-a" and "-oa" as metadump arguments.  Some corruptions have
appeared, but I don't yet know if they're regressions or not.
(i.e. the filesystem was intact but the restored metadump was not.)

I'll let the run finish and re-run the failed tests w/ stock code and see
what I find.

-Eric

[PATCH 3/5] xfs_metadump: Zap freeindex blocks in directory inodes

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
---
 db/metadump.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index be7cf360..d4c751c0 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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();
-- 
2.14.5

Re: [PATCH 3/5] xfs_metadump: Zap freeindex blocks in directory inodes

[Darrick J. Wong]

On Mon, Nov 05, 2018 at 10:31:43PM +0100, Stefan Ring wrote:
> Signed-off-by: Stefan Ring <stefanrin@gmail.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  db/metadump.c | 46 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index be7cf360..d4c751c0 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -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();
> -- 
> 2.14.5
> 

[PATCH 4/5] xfs_metadump: Zap unused space in inode btrees

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
---
 db/metadump.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/db/metadump.c b/db/metadump.c
index d4c751c0..59765263 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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;
-- 
2.14.5

Re: [PATCH 4/5] xfs_metadump: Zap unused space in inode btrees

[Darrick J. Wong]

On Mon, Nov 05, 2018 at 10:31:44PM +0100, Stefan Ring wrote:
> Signed-off-by: Stefan Ring <stefanrin@gmail.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  db/metadump.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index d4c751c0..59765263 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -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;
> -- 
> 2.14.5
> 

[PATCH 5/5] xfs_metadump: Zap dev inodes

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
---
 db/metadump.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/db/metadump.c b/db/metadump.c
index 59765263..57d6bc09 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2269,6 +2269,25 @@ process_inode_data(
 	return 1;
 }
 
+static int
+process_dev_inode(
+	xfs_dinode_t		*dip)
+{
+	if (XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) {
+		if (show_warnings)
+			print_warning("inode %llu has unexpected extents",
+				      (unsigned long long)cur_ino);
+		return 0;
+	} else {
+		if (zero_stale_data) {
+			unsigned int	size = sizeof(xfs_dev_t);
+			memset(XFS_DFORK_DPTR(dip) + size, 0,
+					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();
 
-- 
2.14.5

Re: [PATCH 5/5] xfs_metadump: Zap dev inodes

[Darrick J. Wong]

On Mon, Nov 05, 2018 at 10:31:45PM +0100, Stefan Ring wrote:
> Signed-off-by: Stefan Ring <stefanrin@gmail.com>
> ---
>  db/metadump.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 59765263..57d6bc09 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2269,6 +2269,25 @@ process_inode_data(
>  	return 1;
>  }
>  
> +static int
> +process_dev_inode(
> +	xfs_dinode_t		*dip)
> +{
> +	if (XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) {
> +		if (show_warnings)
> +			print_warning("inode %llu has unexpected extents",
> +				      (unsigned long long)cur_ino);
> +		return 0;
> +	} else {
> +		if (zero_stale_data) {
> +			unsigned int	size = sizeof(xfs_dev_t);
> +			memset(XFS_DFORK_DPTR(dip) + size, 0,

Blank line needed between the  variable definition and the first line of
code.  Otherwise looks fine to me...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> +					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();
>  
> -- 
> 2.14.5
> 

Re: [PATCH v5 0/5] Try to squash metadump data leaks

[Stefan Ring]

On Mon, Nov 5, 2018 at 10:32 PM Stefan Ring <stefanrin@gmail.com> wrote:
>
> 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(-)

Is this an appropriate time interval for pinging?

Re: [PATCH v5 0/5] Try to squash metadump data leaks

[Eric Sandeen]

On 12/6/18 11:04 AM, Stefan Ring wrote:
> On Mon, Nov 5, 2018 at 10:32 PM Stefan Ring <stefanrin@gmail.com> wrote:
>>
>> 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(-)
> 
> Is this an appropriate time interval for pinging?
> 

Yeah, this is back on my radar today as I try to assemble an xfsprogs-4.20.

Sorry for the delay, and thanks for the work.

-Eric

[PATCH v6 0/5] Try to squash metadump data leaks

[Stefan Ring]

After the (hopefully) final round of reviews:

- Changed the control flow logic in patch 2
- Added the newline in patch 5

As a consequence of the former, patch 3 has minimally changed and does
not remove the write: label anymore. I applied Reviewed-by anyway; I
hope that's ok.

Also here, if this is more convenient:
https://github.com/Ringdingcoder/xfsprogs-dev/commits/metadump-stale-data-v6

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 | 132 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 117 insertions(+), 15 deletions(-)

-- 
2.19.2

[PATCH 1/5] xfs_metadump: Extend data zapping to XFS_DIR{2,3}_LEAFN_MAGIC blocks

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
---
 db/metadump.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index cc2ae9af..97d2a490 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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;
 	}
 }
 
-- 
2.19.2

[PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Stefan Ring]

Using basically the same code as in process_single_fsb_objects.

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
---
 db/metadump.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 97d2a490..45705685 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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();
-- 
2.19.2

Re: [PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Eric Sandeen]

On 1/7/19 2:13 PM, Stefan Ring wrote:
> Using basically the same code as in process_single_fsb_objects.
> 
> Signed-off-by: Stefan Ring <stefanrin@gmail.com>

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

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  db/metadump.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 97d2a490..45705685 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -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();
> 

Re: [PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Stefan Ring]

On Fri, Jan 11, 2019 at 6:31 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>
> On 1/7/19 2:13 PM, Stefan Ring wrote:
> > Using basically the same code as in process_single_fsb_objects.
> >
> > Signed-off-by: Stefan Ring <stefanrin@gmail.com>
>
> thanks, the change here to goto write: looks good to me.
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

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>.

Re: [PATCH 2/5] xfs_metadump: Zap multi fsb blocks

[Eric Sandeen]

On 1/14/19 11:42 AM, Stefan Ring wrote:
> On Fri, Jan 11, 2019 at 6:31 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>>
>> On 1/7/19 2:13 PM, Stefan Ring wrote:
>>> Using basically the same code as in process_single_fsb_objects.
>>>
>>> Signed-off-by: Stefan Ring <stefanrin@gmail.com>
>>
>> thanks, the change here to goto write: looks good to me.
>>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> 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>.
> 

I don't think so.  Pretty sure the failures I ran into with my extended
xfstests harness are actually pre-existing, and Carlos is looking at the
root cause.  I'll give it another once-over but I hope/plan to include this
in xfsprogs-4.20 now.

Apologies for the long wait, I can't tell you how much I appreciate
your effort on this.  Thank you!

-Eric

[PATCH 3/5] xfs_metadump: Zap freeindex blocks in directory inodes

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/metadump.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 45705685..edc288ce 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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 {
@@ -1927,8 +1963,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 {
-- 
2.19.2

[PATCH 4/5] xfs_metadump: Zap unused space in inode btrees

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/metadump.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/db/metadump.c b/db/metadump.c
index edc288ce..3994c4f4 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2176,6 +2176,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;
-- 
2.19.2

[PATCH 5/5] xfs_metadump: Zap dev inodes

[Stefan Ring]

Signed-off-by: Stefan Ring <stefanrin@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/metadump.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/db/metadump.c b/db/metadump.c
index 3994c4f4..b02e6074 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2272,6 +2272,26 @@ process_inode_data(
 	return 1;
 }
 
+static int
+process_dev_inode(
+	xfs_dinode_t		*dip)
+{
+	if (XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) {
+		if (show_warnings)
+			print_warning("inode %llu has unexpected extents",
+				      (unsigned long long)cur_ino);
+		return 0;
+	} else {
+		if (zero_stale_data) {
+			unsigned int	size = sizeof(xfs_dev_t);
+
+			memset(XFS_DFORK_DPTR(dip) + size, 0,
+					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.
@@ -2324,7 +2344,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();
 
-- 
2.19.2