On Wed, May 04, 2016 at 03:42:38PM -0500, Eric Sandeen wrote:
> A metadump is composed of many metablocks, which have the format:
>
> [header|indices][ ... disk sectors ... ]
>
> where "disk sectors" are BBSIZE (512) blocks, and the (indices)
> indicate where those disk sectors should land in the restored
> image.
>
> The header+indices fit within a single BBSIZE sector, and as such
> the number of indices is limited to:
>
> num_indices = (BBSIZE - sizeof(xfs_metablock_t)) / sizeof(__be64);
>
> In practice, this works out to 63 indices; sadly 64 are required
> to store a 32k metadata chunk, if the filesystem was created with
> XFS_MAX_SECTORSIZE. This leads to more sadness later on, as we
> index past arrays etc.
>
> For now, just refuse to create a metadump from a 32k sector
> filesystem; that's largely just theoretical at this point anyway.
>
> Also check this on mdrestore, and check the lower bound as well;
> the AFL fuzzer showed that interesting things happen when the
> metadump image claims to contain a sector size of 0.
>
> Oh, and spell "indices" correctly while we're at it.
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
Couple nits...
>
> diff --git a/db/metadump.c b/db/metadump.c
> index 26a3bd5..f366f86 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -66,7 +66,7 @@ static xfs_metablock_t *metablock; /* header + index + buffers */
> static __be64 *block_index;
> static char *block_buffer;
>
> -static int num_indicies;
> +static int num_indices;
> static int cur_index;
>
> static xfs_ino_t cur_ino;
> @@ -147,7 +147,7 @@ print_progress(const char *fmt, ...)
> * A complete dump file will have a "zero" entry in the last index block,
> * even if the dump is exactly aligned, the last index will be full of
> * zeros. If the last index entry is non-zero, the dump is incomplete.
> - * Correspondingly, the last chunk will have a count < num_indicies.
> + * Correspondingly, the last chunk will have a count < num_indices.
> *
> * Return 0 for success, -1 for failure.
> */
> @@ -164,7 +164,7 @@ write_index(void)
> return -errno;
> }
>
> - memset(block_index, 0, num_indicies * sizeof(__be64));
> + memset(block_index, 0, num_indices * sizeof(__be64));
> cur_index = 0;
> return 0;
> }
> @@ -184,7 +184,7 @@ write_buf_segment(
> for (i = 0; i < len; i++, off++, data += BBSIZE) {
> block_index[cur_index] = cpu_to_be64(off);
> memcpy(&block_buffer[cur_index << BBSHIFT], data, BBSIZE);
> - if (++cur_index == num_indicies) {
> + if (++cur_index == num_indices) {
> ret = write_index();
> if (ret)
> return -EIO;
> @@ -2656,7 +2656,19 @@ metadump_f(
>
> block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));
> block_buffer = (char *)metablock + BBSIZE;
> - num_indicies = (BBSIZE - sizeof(xfs_metablock_t)) / sizeof(__be64);
> + num_indices = (BBSIZE - sizeof(xfs_metablock_t)) / sizeof(__be64);
> +
> + /*
> + * A metadump block can hold at most num_indices of BBSIZE sectors;
> + * do not try to dump a filesystem with a sector size which does not
> + * fit within num_indices (i.e. within a single metablock).
> + */
> + if (mp->m_sb.sb_sectsize > num_indices * BBSIZE) {
> + print_warning("Cannot dump filesystem with sector size %u",
> + mp->m_sb.sb_sectsize);
> + return 0;
> + }
> +
I know we're exiting, but we should probably free metablock here (as the
subsequent error checks do).
> cur_index = 0;
> start_iocur_sp = iocur_sp;
>
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 70a160c..3ac3e89 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -60,7 +60,7 @@ perform_restore(
> __be64 *block_index;
> char *block_buffer;
> int block_size;
> - int max_indicies;
> + int max_indices;
> int cur_index;
> int mb_count;
> xfs_metablock_t tmb;
> @@ -80,14 +80,14 @@ perform_restore(
> fatal("specified file is not a metadata dump\n");
>
> block_size = 1 << tmb.mb_blocklog;
> - max_indicies = (block_size - sizeof(xfs_metablock_t)) / sizeof(__be64);
> + max_indices = (block_size - sizeof(xfs_metablock_t)) / sizeof(__be64);
>
> - metablock = (xfs_metablock_t *)calloc(max_indicies + 1, block_size);
> + metablock = (xfs_metablock_t *)calloc(max_indices + 1, block_size);
> if (metablock == NULL)
> fatal("memory allocation failure\n");
>
> mb_count = be16_to_cpu(tmb.mb_count);
> - if (mb_count == 0 || mb_count > max_indicies)
> + if (mb_count == 0 || mb_count > max_indices)
> fatal("bad block count: %u\n", mb_count);
>
> block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));
> @@ -109,6 +109,16 @@ perform_restore(
> if (sb.sb_magicnum != XFS_SB_MAGIC)
> fatal("bad magic number for primary superblock\n");
>
> + /*
> + * Normally the upper bound would be simply XFS_MAX_SECTORSIZE
> + * but the metadump format has a maximum number of BBSIZE blocks
> + * it can store in a single metablock.
> + */
> + if (sb.sb_sectsize < XFS_MIN_SECTORSIZE ||
> + sb.sb_sectsize > XFS_MAX_SECTORSIZE ||
> + sb.sb_sectsize > max_indices * block_size)
Trailing whitespace above.
With those fixed:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> + fatal("bad sector size %u in metadump image\n", sb.sb_sectsize);
> +
> ((xfs_dsb_t*)block_buffer)->sb_inprogress = 1;
>
> if (is_target_file) {
> @@ -144,7 +154,7 @@ perform_restore(
> be64_to_cpu(block_index[cur_index]) << BBSHIFT,
> strerror(errno));
> }
> - if (mb_count < max_indicies)
> + if (mb_count < max_indices)
> break;
>
> if (fread(metablock, block_size, 1, src_f) != 1)
> @@ -153,7 +163,7 @@ perform_restore(
> mb_count = be16_to_cpu(metablock->mb_count);
> if (mb_count == 0)
> break;
> - if (mb_count > max_indicies)
> + if (mb_count > max_indices)
> fatal("bad block count: %u\n", mb_count);
>
> if (fread(block_buffer, mb_count << tmb.mb_blocklog,
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
