linux-kernel - Re: [PATCH 03/29] fscrypt: Enable partial page encryption

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:   Tue, 15 Nov 2016 10:31:40 -0800
From:   Eric Biggers <ebiggers@...gle.com>
To:     Richard Weinberger <richard@....at>
Cc:     linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, dedekind1@...il.com,
        adrian.hunter@...el.com, tytso@....edu, jaegeuk@...nel.org,
        david@...ma-star.at, wd@...x.de, sbabic@...x.de,
        dengler@...utronix.de, mhalcrow@...gle.com, hch@...radead.org
Subject: Re: [PATCH 03/29] fscrypt: Enable partial page encryption

On Sun, Nov 13, 2016 at 10:20:46PM +0100, Richard Weinberger wrote:
> From: David Gstir <david@...ma-star.at>
> 
> Not all filesystems work on full pages, thus we should allow them to
> hand partial pages to fscrypt for en/decryption.
> 
> Signed-off-by: David Gstir <david@...ma-star.at>
> Signed-off-by: Richard Weinberger <richard@....at>
> ---
>  fs/crypto/crypto.c       | 42 ++++++++++++++++++++++++++----------------
>  fs/ext4/inode.c          |  6 ++++--
>  fs/ext4/page-io.c        |  2 +-
>  fs/f2fs/data.c           |  2 ++
>  include/linux/fscrypto.h | 16 +++++++++++-----
>  5 files changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 222a70520565..e170aa05011d 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -149,6 +149,7 @@ typedef enum {
>  static int do_page_crypto(struct inode *inode,
>  			fscrypt_direction_t rw, pgoff_t index,
>  			struct page *src_page, struct page *dest_page,
> +			unsigned int src_len, unsigned int src_offset,
>  			gfp_t gfp_flags)

The naming of 'src_len' and 'src_offset', and 'plaintext_len' and
'plaintext_offset' below, is misleading because the length and offset actually
apply to the destination too.  Shouldn't they be 'len' and 'offset', or 'len'
and 'offs' like fscrypt_decrypt_page()?

I'm also a little concerned that users will mix up the src_len and src_offset
arguments and end up "encrypting" 0 bytes at offset PAGE_SIZE.  Adding a
'BUG_ON(len == 0)' may be appropriate.

>  /**
>   * fscypt_encrypt_page() - Encrypts a page
> - * @inode:          The inode for which the encryption should take place
> - * @plaintext_page: The page to encrypt. Must be locked.
> - * @gfp_flags:      The gfp flag for memory allocation
> + * @inode:            The inode for which the encryption should take place
> + * @plaintext_page:   The page to encrypt. Must be locked.
> + * @plaintext_len:    Length of plaintext within page
> + * @plaintext_offset: Offset of plaintext within page
> + * @gfp_flags:        The gfp flag for memory allocation
>   *
>   * Encrypts plaintext_page using the ctx encryption context. If
>   * the filesystem supports it, encryption is performed in-place, otherwise a
> @@ -229,13 +232,17 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx *ctx, gfp_t gfp_flags)
>   * error value or NULL.
>   */
>  struct page *fscrypt_encrypt_page(struct inode *inode,
> -				struct page *plaintext_page, gfp_t gfp_flags)
> +				struct page *plaintext_page,
> +				unsigned int plaintext_len,
> +				unsigned int plaintext_offset,
> +				gfp_t gfp_flags)
> +
>  {
>  	struct fscrypt_ctx *ctx;
>  	struct page *ciphertext_page = plaintext_page;
>  	int err;
>  
> -	BUG_ON(!PageLocked(plaintext_page));
> +	BUG_ON(plaintext_len % FS_CRYPTO_BLOCK_SIZE != 0);

What is going on with PageLocked()?  Is it still a requirement?  If not the
function comment needs to be fixed.

> -int fscrypt_decrypt_page(struct inode *inode, struct page *page)
> +int fscrypt_decrypt_page(struct inode *inode, struct page *page,
> +			unsigned int len, unsigned int offs)
>  {
> -	BUG_ON(!PageLocked(page));
> -
> -	return do_page_crypto(inode, FS_DECRYPT, page->index, page, page,
> +	return do_page_crypto(inode, FS_DECRYPT, page->index, page, page, len, offs,
>  			GFP_NOFS);
>  }

Same with PageLocked().  Is it still a requirement?  If not the function comment
needs to be fixed.

Eric