On Jul 17, 2019, at 7:13 PM, 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 xattr for all encrypted inodes.  If an encrypted inode has
>  a missing or clearly invalid xattr, offer to clear the inode.
> 
> - During pass 2 (directory structure check), 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>
> ---
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index 2d359b38..10dcb582 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -135,6 +135,22 @@ struct dx_dirblock_info {
> #define DX_FLAG_FIRST		4
> #define DX_FLAG_LAST		8
> 
> +/*
> + * The encrypted file information structure; stores information for files which
> + * are encrypted.
> + */
> +struct encrypted_file {
> +	ext2_ino_t ino;
> +	void *xattr_value;
> +	size_t xattr_size;
> +};

This structure is pretty memory inefficient.  4 byte ino, 8 bytes pointer, then a
8 byte size. I don't think that we need a full size_t to store a valid xattr size,
given that is limited to 64KB currently, while size_t is an unsigned long.

It would save 8 bytes per inode to rearrange these, and add a unique prefix to make
the fields easier to find:

struct e2fsck_encrypted_file {
	ext2_ino_t   eef_ino;
	unsigned int eef_xattr_size;
	void        *eef_xattr_value;
};

> +struct encrypted_files {
> +	size_t count;
> +	size_t capacity;
> +	struct encrypted_file *files;
> +};

Searching for "encrypted_file" vs. "encrypted_files" is not great.  Maybe
"e2fsck_encrypted_(file_)list" or "e2fsck_encrypted_(file_)array"?  As above,
better to have a prefix for these structure fields, like "eel_" or "eea_".

> +int add_encrypted_file(e2fsck_t ctx, struct problem_context *pctx)
> +{
> +	pctx->errcode = get_encryption_xattr(ctx, ino, &file->xattr_value,
> +					     &file->xattr_size);
> +	if (pctx->errcode) {
> +		if (fix_problem(ctx, PR_1_MISSING_ENCRYPTION_XATTR, pctx))
> +			return -1;

At this point, you don't really know if the inode _should_ be encrypted,
or if it is a stray bit flip in the EXT4_ENCRYPT_STATE that resulted in
add_encrypted_file being called.  This results in the inode being deleted,
even though it is possible that it was never encrypted.  This determination
should be made later when the inode's parent directory is known.  Either
the parent also has an encryption flag+xattr and it _should_ have been
encrypted and should be cleared, or the parent doesn't have an encryption
flag+xattr and only the child inode flag should be cleared...

> +	} else if (!possibly_valid_encryption_xattr(file->xattr_value,
> +						    file->xattr_size)) {
> +		ext2fs_free_mem(&file->xattr_value);
> +		file->xattr_size = 0;
> +		if (fix_problem(ctx, PR_1_CORRUPT_ENCRYPTION_XATTR, pctx))
> +			return -1;
> +	}


I can see in this case, where the inode is flagged and the encryption xattr
exists (named "c" if I read correctly?), but is corrupt, then there isn't
much to do for the file.

> @@ -1415,18 +1478,21 @@ skip_checksum:
> 			}
> 
> -		if (encrypted && (dot_state) > 1 &&
> -		    encrypted_check_name(ctx, dirent, &cd->pctx)) {

No need for () around "dot_state".


Cheers, Andreas