Re: [PATCH 04/13] lib/base64: RFC4648-compliant base64 encoding

From: Hannes Reinecke <hare@suse.de>
To: Eric Biggers <ebiggers@kernel.org>
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
Date: Wed, 11 Aug 2021 07:57:33 +0200	[thread overview]
Message-ID: <f3fc5af2-835c-4cb4-4715-9c95a359002c@suse.de> (raw)
In-Reply-To: <YRLP5JuQrF/SJPBt@gmail.com>

On 8/10/21 9:13 PM, Eric Biggers wrote:
> 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.
> 
Hmm. Looking into it.

>> +/**
>> + * 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.
> 

Ok, will be updating.

>> + *
>> + * 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.
> 

Indeed, it should. Will be fixing it up.

>> +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.
> 
Why not?
'lookup_table' is pretty much static, so where't the problem?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme