lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 21 Feb 2018 15:27:34 +0530
From:   Chandan Rajendra <chandan@...ux.vnet.ibm.com>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-fscrypt@...r.kernel.org, tytso@....edu
Subject: Re: [RFC PATCH V2 00/11] Ext4 encryption support for blocksize < pagesize

On Wednesday, February 21, 2018 6:18:21 AM IST Eric Biggers wrote:
> Hi Chandan,
> 
> On Mon, Feb 12, 2018 at 03:13:36PM +0530, Chandan Rajendra wrote:
> > This patchset implements code to support encryption of Ext4 filesystem
> > instances that have blocksize less than pagesize. The patchset has
> > been tested on both ppc64 and x86_64 machines.
> > 
> > Eric, fscrypt_mpage_readpages() (originally, ext4_mpage_readpages())
> > still retains the ability to read non-encrypted file data. Please let
> > me know if the code has to be changed such that
> > fscrypt_mpage_readpages() makes it mandatory for the file's data to be
> > encrypted.
> > 
> > TODO:
> > F2FS and UBIFS code needs to be updated to make use of the newly added
> > fscrypt functions. I will do that in the next version of the patchset.
> > 
> > Changelog:
> > "RFC V1" -> "RFC V2":
> > 1. Ext4's "encryption aware" functionality in fs/ext4/readpage.c has
> >    been moved to fs/crypto/.
> > 2. fscrypt functions have now been renamed to indicate that they work
> >    on blocks rather than pages.
> >    Eric, I have renamed completion_pages() to fscrypt_complete_pages()
> >    rather than to fscrypt_complete_blocks(). This is because we have a
> >    new function fscrypt_complete_block() (which operates on a single
> >    block) and IMHO having the identifier fscrypt_complete_blocks()
> >    which differs from it by just one letter would confuse the reader.
> > 3. ext4_block_write_begin() now clears BH_Uptodate flag when
> >    decryption of boundary blocks fail.
> > 4. fscrypt_encrypt_page() (now renamed to fscrypt_encrypt_block()) is
> >    now split into two functions. fscrypt_prep_ciphertext_page()
> >    allocates and initializes the fscrypt context and the bounce
> >    page. fscrypt_encrypt_block() is limited to encrypting the
> >    filesystem's block.
> > 5. fscrypt_zeroout_range() has been updated to work on blocksize <
> >    pagesize scenario.
> > 6. Documentation/filesystems/fscrypt.rst has been updated to indicate
> >    encryption support for blocksize < pagesize.
> > 
> > Thanks to Eric Biggers for providing review comments for "RFC V1".
> > 
> 
> Thanks for the new version of the patchset.
> 
> I see you decided to move ext4's readpages to fs/crypto/.  Did you also consider
> the other alternatives I had suggested, such as adding an encryption callback to
> the generic mpage_readpages(), or making fscrypt non-modular and then calling it
> directly from mpage_readpages()?  Maybe you did, but I don't see the tradeoffs
> addressed in the patchset at all.  The patches need to explain *why* you're
> doing what you're doing, not just *what* you're doing.

I had glanced through F2FS and UBIFS source code. F2FS has its own version of
mpage_readpage[s] and UBIFS does not use mpage_readpage[s]
functionality. This was the major reason for deciding to not go with the
approach of having a decryption call back passed to mpage_readpage[s].

Apart from the reason of memory being wasted on systems which do not require
files to be encrypted, the previously listed reason of mpage_readpage[s] not
being used by F2FS and UBIFS also played a role is deciding against invoking
fscrypt_decrypt_bio_blocks() from within mpage_readpages().

> 
> (Just a thought: if we did make fscrypt non-modular, we could potentially limit
> what gets pulled in from the crypto API by dropping the 'selects' of the
> specific crypto algorithms, since they aren't hard dependencies.  Also the
> options like CRYPTO_AES and CRYPTO_XTS that are currently being selected
> actually refer to the generic implementations of those algorithms, which usually
> aren't the ones people want to be using since they can be very slow compared to
> architecture-specific versions.)
> 
> It would also be easier to review the patch that moves ext4_mpage_readpages() to
> fs/crypto/ if you split out the behavior change (allowing blocksize < pagesize)
> into a separate patch.  Switching ext4 to use it can be separate from adding it
> as well.  
> 

I will implement this in the next version of the patchset.

> Note that the situation with ext4_block_write_begin() vs __block_write_begin()
> is very similar to ext4_mpage_readpages() vs mpage_readpages().  So I think
> whatever you are doing with readpages (e.g. moving it to fs/crypto/, or adding a
> callback to the generic version) should also be done with __block_write_begin().

I will implement this in the next version of the patchset.

> 
> In general, it would also be helpful if you kept comments up to date.  It makes
> it difficult to read the code when I see fscrypt_encrypt_block(), but the
> comment says fscrypt_encrypt_page() and the documented behavior is very
> different from what it actually does.  fscrypt_prep_ciphertext_page() also
> doesn't have a comment at all.

I will update the comments.

> 
> Updating f2fs and ubifs is important as well so that you can see whether the
> changes actually work for them or not.

Yes, I have started making the changes for F2FS and will make sure to get it 
working for UBIFS before posting the next version of the patchset.


Eric, Thanks once again for the review comments.


-- 
chandan

Powered by blists - more mailing lists