Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 10 Aug 2021 12:13:40 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Hannes Reinecke <hare@suse.de>
Cc:     Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>,
        Keith Busch <keith.busch@wdc.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S . Miller" <davem@davemloft.net>,
        linux-nvme@lists.infradead.org, linux-crypto@vger.kernel.org
Subject: Re: [PATCH 04/13] lib/base64: RFC4648-compliant base64 encoding
Message-ID: <YRLP5JuQrF/SJPBt@gmail.com>
References: <20210810124230.12161-1-hare@suse.de>
 <20210810124230.12161-5-hare@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210810124230.12161-5-hare@suse.de>
Precedence: bulk

On Tue, Aug 10, 2021 at 02:42:21PM +0200, Hannes Reinecke wrote:
> Add RFC4648-compliant base64 encoding and decoding routines.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  include/linux/base64.h |  16 ++++++
>  lib/Makefile           |   2 +-
>  lib/base64.c           | 115 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 132 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/base64.h
>  create mode 100644 lib/base64.c
> 
> diff --git a/include/linux/base64.h b/include/linux/base64.h
> new file mode 100644
> index 000000000000..660d4cb1ef31
> --- /dev/null
> +++ b/include/linux/base64.h
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * base64 encoding, lifted from fs/crypto/fname.c.
> + */

As I mentioned previously, please make it very clear which variant of Base64 it
is (including whether padding is included and required or not), and update all
the comments accordingly.  I've done so in fs/crypto/fname.c with
https://lkml.kernel.org/r/20210718000125.59701-1-ebiggers@kernel.org.  It would
probably be best to start over with a copy of that and modify it accordingly to
implement the desired variant of Base64.

> +/**
> + * base64_decode() - base64-decode some bytes
> + * @src: the base64-encoded string to decode
> + * @len: number of bytes to decode
> + * @dst: (output) the decoded bytes.

"@len: number of bytes to decode" is ambiguous as it could refer to either @src
or @dst.  I've fixed this in the fs/crypto/fname.c version.

> + *
> + * Decodes the base64-encoded bytes @src according to RFC 4648.
> + *
> + * Return: number of decoded bytes
> + */

Shouldn't this return an error if the string is invalid?  Again, see the latest
fs/crypto/fname.c version.

> +int base64_decode(const char *src, int len, u8 *dst)
> +{
> +        int i, bits = 0, pad = 0;
> +        u32 ac = 0;
> +        size_t dst_len = 0;
> +
> +        for (i = 0; i < len; i++) {
> +                int c, p = -1;
> +
> +                if (src[i] == '=') {
> +                        pad++;
> +                        if (i + 1 < len && src[i + 1] == '=')
> +                                pad++;
> +                        break;
> +                }
> +                for (c = 0; c < strlen(lookup_table); c++) {

strlen() shouldn't be used in a loop condition like this.

- Eric