Message-ID: <20190719235852.GI1422@gmail.com>
Date:   Fri, 19 Jul 2019 16:58:53 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux-fscrypt@...r.kernel.org
Subject: Re: [PATCH] e2fsck: check for consistent encryption policies

On Thu, Jul 18, 2019 at 12:25:42AM -0600, Andreas Dilger wrote:
> On Jul 17, 2019, at 7:13 PM, Eric Biggers <ebiggers@...nel.org> wrote:
> > 
> > From: Eric Biggers <ebiggers@...gle.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@...gle.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,

Yes, it can be reduced to 16 bytes.  Probably we should go with the 8 bytes
(ino, id) as you suggested elsewhere, though.

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

I can change it to *_file_list.  However, did you see that all the other structs
in e2fsck/e2fsck.h besides e2fsck_struct already have names like:

	struct dir_info
	struct dx_dir_info
	struct dx_dirblock_info
	struct resource_track
	struct ea_refcount
	struct extent_tree_level
	struct extent_tree_info

So for consistency, I'm not sure the "e2fsck_" prefix is warranted.

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

How about simply clearing the encrypt flag instead in this case?

That does the right thing in the bit flip case.  It also does the right thing
when the file is in an encrypted directory (so it's supposed to be encrypted),
as then the file will be deleted during the encryption policy check in pass 2
due to PR_2_UNENCRYPTED_FILE.

It wouldn't properly handle the rare case where a new encryption policy was just
set on a top level directory, and encrypted entries were already created in it
before the encryption xattr was persisted.  The directory would be marked as
unencrypted, not deleted as expected.  Though this should be a really rare case,
and hard to distinguish from a bitflip anyway, so probably not worth worrying
about...  (And it could also go the other way: xattr was persisted but not the
encrypt flag, which would be inconvenient to check for.)

> 
> > +	} 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".

This patch actually removes those unnecessary ().

- Eric

Powered by blists - more mailing lists