Re: [RFC PATCH 05/14] lib: lift fscrypt base64 conversion into lib/

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Aug 21, 2020 at 02:28:04PM -0400, Jeff Layton wrote:
> Once we allow encrypted filenames we'll end up with names that may have
> illegal characters in them (embedded '\0' or '/'), or characters that
> aren't printable.
> 
> It'll be safer to use strings that are printable. It turns out that the
> MDS doesn't really care about the length of filenames, so we can just
> base64 encode and decode filenames before writing and reading them.
> 
> Lift the base64 implementation that's in fscrypt into lib/. Make fscrypt
> select it when it's enabled.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/crypto/Kconfig      |  1 +
>  fs/crypto/fname.c      | 59 +----------------------------------
>  include/linux/base64.h | 11 +++++++
>  lib/Kconfig            |  3 ++
>  lib/Makefile           |  1 +
>  lib/base64.c           | 71 ++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 88 insertions(+), 58 deletions(-)
>  create mode 100644 include/linux/base64.h
>  create mode 100644 lib/base64.c

You need to be careful here because there are many subtly different variants of
base64.  The Wikipedia article is a good reference for this:
https://en.wikipedia.org/wiki/Base64

For example, most versions of base64 use [A-Za-z0-9+/].  However that's *not*
what fs/crypto/fname.c uses, since it needs the encoded strings to be valid
filenames, and '/' isn't a valid character in filenames.  Therefore,
fs/crypto/fname.c uses ',' instead of '/'.

It happens that's probably what ceph needs too.  However, other kernel
developers who come across a very generic-sounding "lib/base64.c" might expect
it to implement a more common version of base64.

Also, some versions of base64 pad the encoded string with "=" whereas others
don't.  The fs/crypto/fname.c implementation doesn't use padding.

So if you're going to make a generic base64 library, you at least need to be
very clear about exactly what version of base64 is meant.

(FWIW, the existing use of base64 in fs/crypto/fname.c isn't part of a stable
API.  So it can still be changed to something else, as long as the encoding
doesn't use the '/' or '\0' characters.)

- Eric