Message-ID: <87zglgoi1e.fsf@brahms.olymp>
Date:   Wed, 23 Mar 2022 14:33:17 +0000
From:   Luís Henriques <lhenriques@...e.de>
To:     Eric Biggers <ebiggers@...gle.com>
Cc:     Jeff Layton <jlayton@...nel.org>, idryomov@...il.com,
        xiubli@...hat.com, ceph-devel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-fscrypt@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v11 02/51] fscrypt: export fscrypt_base64url_encode
 and fscrypt_base64url_decode

Hi Eric,

Jeff Layton <jlayton@...nel.org> writes:

> Ceph is going to add fscrypt support, but we still want encrypted
> filenames to be composed of printable characters, so we can maintain
> compatibility with clients that don't support fscrypt.
>
> We could just adopt fscrypt's current nokey name format, but that is
> subject to change in the future, and it also contains dirhash fields
> that we don't need for cephfs. Because of this, we're going to concoct
> our own scheme for encoding encrypted filenames. It's very similar to
> fscrypt's current scheme, but doesn't bother with the dirhash fields.
>
> The ceph encoding scheme will use base64 encoding as well, and we also
> want it to avoid characters that are illegal in filenames. Export the
> fscrypt base64 encoding/decoding routines so we can use them in ceph's
> fscrypt implementation.
>
> Acked-by: Eric Biggers <ebiggers@...gle.com>
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  fs/crypto/fname.c       | 8 ++++----
>  include/linux/fscrypt.h | 5 +++++
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index a9be4bc74a94..1e4233c95005 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -182,8 +182,6 @@ static int fname_decrypt(const struct inode *inode,
>  static const char base64url_table[65] =
>  	"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
>  
> -#define FSCRYPT_BASE64URL_CHARS(nbytes)	DIV_ROUND_UP((nbytes) * 4, 3)
> -
>  /**
>   * fscrypt_base64url_encode() - base64url-encode some binary data
>   * @src: the binary data to encode
> @@ -198,7 +196,7 @@ static const char base64url_table[65] =
>   * Return: the length of the resulting base64url-encoded string in bytes.
>   *	   This will be equal to FSCRYPT_BASE64URL_CHARS(srclen).
>   */
> -static int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst)
> +int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst)

I know you've ACK'ed this patch already, but I was wondering if you'd be
open to change these encode/decode interfaces so that they could be used
for non-url base64 too.

My motivation is that ceph has this odd limitation where snapshot names
can not start with the '_' character.  And I've an RFC that adds snapshot
names encryption support which, unfortunately, can end up starting with
this char after base64 encoding.

So, my current proposal is to use a different encoding table.  I was
thinking about the IMAP mailboxes naming which uses '+' and ',' instead of
the '-' and '_', but any other charset would be OK (except those that
include '/' of course).  So, instead of adding yet another base64
implementation to the kernel, I was wondering if you'd be OK accepting a
patch to add an optional arg to these encoding/decoding functions to pass
an alternative table.  Or, if you'd prefer, keep the existing interface
but turning these functions into wrappers to more generic functions.

Obviously, Jeff, please feel free to comment too if you have any reserves
regarding this approach.

Cheers,
-- 
Luís

>  {
>  	u32 ac = 0;
>  	int bits = 0;
> @@ -217,6 +215,7 @@ static int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst)
>  		*cp++ = base64url_table[(ac << (6 - bits)) & 0x3f];
>  	return cp - dst;
>  }
> +EXPORT_SYMBOL_GPL(fscrypt_base64url_encode);
>  
>  /**
>   * fscrypt_base64url_decode() - base64url-decode a string
> @@ -233,7 +232,7 @@ static int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst)
>   * Return: the length of the resulting decoded binary data in bytes,
>   *	   or -1 if the string isn't a valid base64url string.
>   */
> -static int fscrypt_base64url_decode(const char *src, int srclen, u8 *dst)
> +int fscrypt_base64url_decode(const char *src, int srclen, u8 *dst)
>  {
>  	u32 ac = 0;
>  	int bits = 0;
> @@ -256,6 +255,7 @@ static int fscrypt_base64url_decode(const char *src, int srclen, u8 *dst)
>  		return -1;
>  	return bp - dst;
>  }
> +EXPORT_SYMBOL_GPL(fscrypt_base64url_decode);
>  
>  bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
>  				  u32 orig_len, u32 max_len,
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 91ea9477e9bd..671181d196a8 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -46,6 +46,9 @@ struct fscrypt_name {
>  /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
>  #define FSCRYPT_SET_CONTEXT_MAX_SIZE	40
>  
> +/* len of resulting string (sans NUL terminator) after base64 encoding nbytes */
> +#define FSCRYPT_BASE64URL_CHARS(nbytes)		DIV_ROUND_UP((nbytes) * 4, 3)
> +
>  #ifdef CONFIG_FS_ENCRYPTION
>  
>  /*
> @@ -305,6 +308,8 @@ void fscrypt_free_inode(struct inode *inode);
>  int fscrypt_drop_inode(struct inode *inode);
>  
>  /* fname.c */
> +int fscrypt_base64url_encode(const u8 *src, int len, char *dst);
> +int fscrypt_base64url_decode(const char *src, int len, u8 *dst);
>  int fscrypt_setup_filename(struct inode *inode, const struct qstr *iname,
>  			   int lookup, struct fscrypt_name *fname);
>  
> -- 
>
> 2.35.1
>

Powered by blists - more mailing lists