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

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2020-08-21 at 17:38 -0700, Eric Biggers wrote:
> 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

Ok, thanks -- that makes sense. We may need to rename this to something
less generic then. I'll plan to do that for the next set.

It may also make more sense to just create fscrypt functions that can
deal with base64-encoded strings instead of binary crypttext.
-- 
Jeff Layton <jlayton@kernel.org>