diff mbox series

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

Message ID 20210810124230.12161-5-hare@suse.de (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series nvme: In-band authentication support | expand

Commit Message

Hannes Reinecke Aug. 10, 2021, 12:42 p.m. UTC
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

Comments

Eric Biggers Aug. 10, 2021, 7:13 p.m. UTC | #1
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
Hannes Reinecke Aug. 11, 2021, 5:57 a.m. UTC | #2
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
diff mbox series

Patch

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.
+ */
+
+#ifndef _LINUX_BASE64_H
+#define _LINUX_BASE64_H
+
+#include <linux/types.h>
+
+#define BASE64_CHARS(nbytes)   DIV_ROUND_UP((nbytes) * 4, 3)
+
+int base64_encode(const u8 *src, int len, char *dst);
+int base64_decode(const char *src, int len, u8 *dst);
+
+#endif /* _LINUX_BASE64_H */
diff --git a/lib/Makefile b/lib/Makefile
index 6d765d5fb8ac..92a428aa0e5f 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -46,7 +46,7 @@  obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
 	 bust_spinlocks.o kasprintf.o bitmap.o scatterlist.o \
 	 list_sort.o uuid.o iov_iter.o clz_ctz.o \
 	 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
-	 percpu-refcount.o rhashtable.o \
+	 percpu-refcount.o rhashtable.o base64.o \
 	 once.o refcount.o usercopy.o errseq.o bucket_locks.o \
 	 generic-radix-tree.o
 obj-$(CONFIG_STRING_SELFTEST) += test_string.o
diff --git a/lib/base64.c b/lib/base64.c
new file mode 100644
index 000000000000..ef120cecceba
--- /dev/null
+++ b/lib/base64.c
@@ -0,0 +1,115 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * base64.c - RFC4648-compliant base64 encoding
+ *
+ * Copyright (c) 2020 Hannes Reinecke, SUSE
+ *
+ * Based on the (slightly non-standard) base64 routines
+ * from fs/crypto/fname.c, but using the RFC-mandated
+ * coding table.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/export.h>
+#include <linux/string.h>
+#include <linux/base64.h>
+
+static const char lookup_table[65] =
+	"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
+
+/**
+ * base64_encode() - base64-encode some bytes
+ * @src: the bytes to encode
+ * @len: number of bytes to encode
+ * @dst: (output) the base64-encoded string.  Not NUL-terminated.
+ *
+ * Encodes the input string using characters from the set [A-Za-z0-9+,].
+ * The encoded string is roughly 4/3 times the size of the input string.
+ *
+ * Return: length of the encoded string
+ */
+int base64_encode(const u8 *src, int len, char *dst)
+{
+	int i, bits = 0;
+	u32 ac = 0;
+	char *cp = dst;
+
+	for (i = 0; i < len; i++) {
+		ac = (ac << 8) | src[i];
+		bits += 8;
+		if (bits < 24)
+			continue;
+		do {
+			bits -= 6;
+			*cp++ = lookup_table[(ac >> bits) & 0x3f];
+		} while (bits);
+		ac = 0;
+	}
+	if (bits) {
+		int more = 0;
+
+		if (bits < 16)
+			more = 2;
+		ac = (ac << (2 + more));
+		bits += (2 + more);
+		do {
+			bits -= 6;
+			*cp++ = lookup_table[(ac >> bits) & 0x3f];
+		} while (bits);
+		*cp++ = '=';
+		if (more)
+			*cp++ = '=';
+	}
+
+	return cp - dst;
+}
+EXPORT_SYMBOL_GPL(base64_encode);
+
+/**
+ * base64_decode() - base64-decode some bytes
+ * @src: the base64-encoded string to decode
+ * @len: number of bytes to decode
+ * @dst: (output) the decoded bytes.
+ *
+ * Decodes the base64-encoded bytes @src according to RFC 4648.
+ *
+ * Return: number of decoded bytes
+ */
+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++) {
+                        if (src[i] == lookup_table[c]) {
+                                p = c;
+                                break;
+                        }
+                }
+                if (p < 0)
+                        break;
+                ac = (ac << 6) | p;
+                bits += 6;
+                if (bits < 24)
+                        continue;
+                while (bits) {
+                        bits -= 8;
+                        dst[dst_len++] = (ac >> bits) & 0xff;
+                }
+                ac = 0;
+        }
+        dst_len -= pad;
+        return dst_len;
+}
+EXPORT_SYMBOL_GPL(base64_decode);