Re: [PATCH v4] e2fsck: check for consistent encryption policies

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 6511 bytes --]

On Sep 9, 2019, at 11:43 AM, Eric Biggers <ebiggers@kernel.org> wrote:
> 
> From: Eric Biggers <ebiggers@google.com>
> 
> By design, the kernel enforces that all files in an encrypted directory
> use the same encryption policy as the directory.  It's not possible to
> violate this constraint using syscalls.  Lookups of files that violate
> this constraint also fail, in case the disk was manipulated.
> 
> But this constraint can also be violated by accidental filesystem
> corruption.  E.g., a power cut when using ext4 without a journal might
> leave new files without the encryption bit and/or xattr.  Thus, it's
> important that e2fsck correct this condition.
> 
> Therefore, this patch makes the following changes to e2fsck:
> 
> - During pass 1 (inode table scan), create a map from inode number to
>  encryption policy for all encrypted inodes.  But it's optimized so
>  that the full xattrs aren't saved but rather only 32-bit "policy IDs",
>  since usually many inodes share the same encryption policy.  Also, if
>  an encryption xattr is missing, offer to clear the encrypt flag.  If
>  an encryption xattr is clearly corrupt, offer to clear the inode.
> 
> - During pass 2 (directory structure check), use the map to verify that
>  all regular files, directories, and symlinks in encrypted directories
>  use the directory's encryption policy.  Offer to clear any directory
>  entries for which this isn't the case.
> 
> Add a new test "f_bad_encryption" to test the new behavior.
> 
> Due to the new checks, it was also necessary to update the existing test
> "f_short_encrypted_dirent" to add an encryption xattr to the test file,
> since it was missing one before, which is now considered invalid.
> 
> Google-Bug-Id: 135138675
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Looks much better.  One minor nit below, but at this point you could add:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> 
> Changes v3 => v4:
> 
> - Save memory in common cases by storing ranges of inodes that share the
>  same encryption policy.
> 
> - Rebased onto latest master branch.
> 
> 
> diff --git a/e2fsck/encrypted_files.c b/e2fsck/encrypted_files.c
> new file mode 100644
> index 00000000..3dc706a7
> --- /dev/null
> +++ b/e2fsck/encrypted_files.c
> @@ -0,0 +1,368 @@
> 
> +/* A range of inodes which share the same encryption policy */
> +struct encrypted_file_range {
> +	ext2_ino_t		first_ino;
> +	ext2_ino_t		last_ino;
> +	__u32			policy_id;
> +};

This seems like a clear win...  As long as we have at least two inodes
in a row with the same policy ID it will take less space than the previous
version of the patch.

> +static int handle_nomem(e2fsck_t ctx, struct problem_context *pctx)
> +{
> +	fix_problem(ctx, PR_1_ALLOCATE_ENCRYPTED_DIRLIST, pctx);
> +	/* Should never get here */
> +	ctx->flags |= E2F_FLAG_ABORT;
> +	return 0;
> +}

It would be useful if the error message for PR_1_ALLOCATE_ENCRYPTED_DIRLIST
printed the actual allocation size that failed, so that the user has some
idea of how much memory would be needed.  The underlying ext2fs_resize_mem()
code doesn't print anything, just returns EXT2_ET_NO_MEMORY.

> +static int append_ino_and_policy_id(e2fsck_t ctx, struct problem_context *pctx,
> +				    ext2_ino_t ino, __u32 policy_id)
> +{
> +	struct encrypted_file_info *info = ctx->encrypted_files;
> +	struct encrypted_file_range *range;
> +
> +	/* See if we can just extend the last range. */
> +	if (info->file_ranges_count > 0) {
> +		range = &info->file_ranges[info->file_ranges_count - 1];
> +
> +		if (ino <= range->last_ino) {
> +			/* Should never get here */
> +			fatal_error(ctx,
> +				    "Encrypted inodes processed out of order");
> +		}
> +
> +		if (ino == range->last_ino + 1 &&
> +		    policy_id == range->policy_id) {
> +			range->last_ino++;
> +			return 0;
> +		}
> +	}
> +	/* Nope, a new range is needed. */
> +
> +	if (info->file_ranges_count == info->file_ranges_capacity) {
> +		/* Double the capacity by default. */
> +		size_t new_capacity = info->file_ranges_capacity * 2;
> +
> +		/* ... but go from 0 to 128 right away. */
> +		if (new_capacity < 128)
> +			new_capacity = 128;
> +
> +		/* We won't need more than the filesystem's inode count. */
> +		if (new_capacity > ctx->fs->super->s_inodes_count)
> +			new_capacity = ctx->fs->super->s_inodes_count;
> +
> +		/* To be safe, ensure the capacity really increases. */
> +		if (new_capacity < info->file_ranges_capacity + 1)
> +			new_capacity = info->file_ranges_capacity + 1;

Not sure how this could happen (more inodes than s_inodes_count?), but
better safe than sorry I guess?

> +		if (ext2fs_resize_mem(info->file_ranges_capacity *
> +					sizeof(*range),
> +				      new_capacity * sizeof(*range),
> +				      &info->file_ranges) != 0)
> +			return handle_nomem(ctx, pctx);

This is the only thing that gives me pause, potentially having a huge
allocation, but I think the RLE encoding of entries and the fact we
have overwhelmingly 64-bit CPUs means we could still run with swap
(on an internal NVMe M.2 device) if really needed.  A problem to fix
if it ever actually rears its head, so long as there is a decent error
message printed.

> +/*
> + * Find the ID of an inode's encryption policy, using the information saved
> + * earlier.
> + *
> + * If the inode is encrypted, returns the policy ID or
> + * UNRECOGNIZED_ENCRYPTION_POLICY.  Else, returns NO_ENCRYPTION_POLICY.
> + */
> +__u32 find_encryption_policy(e2fsck_t ctx, ext2_ino_t ino)
> +{
> +	const struct encrypted_file_info *info = ctx->encrypted_files;
> +	size_t l, r;
> +
> +	if (info == NULL)
> +		return NO_ENCRYPTION_POLICY;
> +	l = 0;
> +	r = info->file_ranges_count;
> +	while (l < r) {
> +		size_t m = l + (r - l) / 2;

Using the RLE encoding for the entries should also speed up searching
here considerably.  In theory, for a single-user Android filesystem
there might only be one or two entries here.  It would be interesting
to run this on some of your filesystems to see what the average count
of inodes per entry is.

> +		const struct encrypted_file_range *range =
> +			&info->file_ranges[m];
> +
> +		if (ino < range->first_ino)
> +			r = m;
> +		else if (ino > range->last_ino)
> +			l = m + 1;
> +		else
> +			return range->policy_id;
> +	}
> +	return NO_ENCRYPTION_POLICY;
> +}


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]