On Jun 28, 2018, at 7:59 AM, Lukas Czerner <lczerner@xxxxxxxxxx> wrote:
>
> We do not allow initialized blocks to exist past i_size as this could
> lead to stale data exposure.
I don't think this is any more dangerous than uninitialized bytes beyond
i_size in the same block, or as a hole in the middle of the file exposing
stale data. That is to say, there is some risk, but not any worse. The
converse is true also - extending i_size to the end of the allocated blocks
will not only expose that data, but in some cases make applications think
the file is corrupted because there are now NULs at the end of the file.
If we think that the existing f_eofblocks test covers the same, then I'm
OK with removing this test. However, the one nice thing about the new test
is that it is generated by test scripts rather than being a binary blob,
so it is more clear what is being tested.
Cheers, Andreas
>
> Remove test f_pgsize_gt_blksize because it is testing for the scenario
> that not allowed. f_eofblocks is already testing for this.
>
> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> ---
> e2fsck/pass1.c | 12 ++----------
> tests/f_eofblocks/expect.1 | 2 ++
> tests/f_pgsize_gt_blksize/expect.1 | 7 -------
> tests/f_pgsize_gt_blksize/expect.2 | 7 -------
> tests/f_pgsize_gt_blksize/name | 1 -
> tests/f_pgsize_gt_blksize/script | 18 ------------------
> 6 files changed, 4 insertions(+), 43 deletions(-)
> delete mode 100644 tests/f_pgsize_gt_blksize/expect.1
> delete mode 100644 tests/f_pgsize_gt_blksize/expect.2
> delete mode 100644 tests/f_pgsize_gt_blksize/name
> delete mode 100644 tests/f_pgsize_gt_blksize/script
>
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index f1fa5d94..461f5fb6 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -3448,16 +3448,8 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
>
> size = EXT2_I_SIZE(inode);
> if ((pb.last_init_lblock >= 0) &&
> - /* if size is smaller than expected by the block count,
> - * allow allocated blocks to end of PAGE_SIZE.
> - * last_init_lblock is the last in-use block, so it is
> - * the minimum expected file size, but +1 because it is
> - * the base-zero block number and not the block count. */
> - (size < (__u64)pb.last_init_lblock * fs->blocksize) &&
> - ((pb.last_init_lblock + 1) / blkpg * blkpg !=
> - (pb.last_init_lblock + 1) ||
> - size < (__u64)(pb.last_init_lblock & ~(blkpg - 1)) *
> - fs->blocksize))
> + /* Do not allow initialized allocated blocks past i_size*/
> + (size < (__u64)pb.last_init_lblock * fs->blocksize))
> bad_size = 3;
> else if (!(extent_fs && (inode->i_flags & EXT4_EXTENTS_FL)) &&
> size > ext2_max_sizes[fs->super->s_log_block_size])
> diff --git a/tests/f_eofblocks/expect.1 b/tests/f_eofblocks/expect.1
> index 34222480..f224b7da 100644
> --- a/tests/f_eofblocks/expect.1
> +++ b/tests/f_eofblocks/expect.1
> @@ -1,4 +1,6 @@
> Pass 1: Checking inodes, blocks, and sizes
> +Inode 30, i_size is 2048, should be 4096. Fix? yes
> +
> Inode 31, i_size is 2048, should be 6144. Fix? yes
>
> Pass 2: Checking directory structure
> diff --git a/tests/f_pgsize_gt_blksize/expect.1 b/tests/f_pgsize_gt_blksize/expect.1
> deleted file mode 100644
> index c00f5db5..00000000
> --- a/tests/f_pgsize_gt_blksize/expect.1
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -Pass 1: Checking inodes, blocks, and sizes
> -Pass 2: Checking directory structure
> -Pass 3: Checking directory connectivity
> -Pass 4: Checking reference counts
> -Pass 5: Checking group summary information
> -test_filesys: 12/32 files (0.0% non-contiguous), 40/100 blocks
> -Exit status is 0
> diff --git a/tests/f_pgsize_gt_blksize/expect.2 b/tests/f_pgsize_gt_blksize/expect.2
> deleted file mode 100644
> index c00f5db5..00000000
> --- a/tests/f_pgsize_gt_blksize/expect.2
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -Pass 1: Checking inodes, blocks, and sizes
> -Pass 2: Checking directory structure
> -Pass 3: Checking directory connectivity
> -Pass 4: Checking reference counts
> -Pass 5: Checking group summary information
> -test_filesys: 12/32 files (0.0% non-contiguous), 40/100 blocks
> -Exit status is 0
> diff --git a/tests/f_pgsize_gt_blksize/name b/tests/f_pgsize_gt_blksize/name
> deleted file mode 100644
> index 3aa02027..00000000
> --- a/tests/f_pgsize_gt_blksize/name
> +++ /dev/null
> @@ -1 +0,0 @@
> -PAGE_SIZE larger than blocksize with hole at end
> diff --git a/tests/f_pgsize_gt_blksize/script b/tests/f_pgsize_gt_blksize/script
> deleted file mode 100644
> index 422b83ae..00000000
> --- a/tests/f_pgsize_gt_blksize/script
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -SKIP_GUNZIP="true"
> -
> -touch $TMPFILE
> -$MKE2FS -N 32 -F -o Linux -b 1024 $TMPFILE 100 > /dev/null 2>&1
> -
> -DATA_FILE=$(mktemp ${TMPDIR:-/tmp}/e2fsprogs-zerodata.XXXXXX)
> -dd if=$TEST_BITS of=$DATA_FILE bs=1k count=16 > /dev/null 2>&1
> -$DEBUGFS -w $TMPFILE << EOF > /dev/null 2>&1
> -write $DATA_FILE foo
> -set_inode_field foo size 13000
> -q
> -EOF
> -
> -. $cmd_dir/run_e2fsck
> -
> -rm -f $DATA_FILE
> -
> -unset DATA_FILE
> --
> 2.17.1
>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
