[PATCH] cipher: RFC on an API for a cipher utility infrastructure

[PATCH] cipher: RFC on an API for a cipher utility infrastructure

[Tomasz Bursztyka]

[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]

---

Hi guys,

Here is a quick proposal on the cipher infra for ell. I followed what has
been done with checksums in ell, with obvious differences due to the ciphers
themselves. (2 different actions: encrypt/decrypt and the presence of a key)

I have open issues here:

l_cipher_reset() on its own might not be useful, should I add the possibilty
to change the key as well? Unless there is no cipher state to reset, and then
it's better not having such function. (I am not aware of the kernel cipher 
internals, related to some state there so if one can enlight the issue here)

Alos, about encrypt/decrypt. Currently I am proposing to have those as they
will send the data, and we get it back via get_data() (function name it not
that great btw).
So:

- is having only encrypt/decrypt an option? (so send/recv would be done in
each functions). Though 2 blocking functions at a time... not great.

- or, what about enabling async access? Unless I miss something about the
crypto layer in the kernel, here the socket is blocking. So we could be
stalled at some point. checksum does the same, is there any plan to make it
async actually? like a function l_cipher_enable_async(), which would call the
right fcntl() and return the socket which we could use in the event loop.

 ell/cipher.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 ell/cipher.h

diff --git a/ell/cipher.h b/ell/cipher.h
new file mode 100644
index 0000000..2a0ca2f
--- /dev/null
+++ b/ell/cipher.h
@@ -0,0 +1,52 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2015  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __ELL_CIPHER_H
+#define __ELL_CIPHER_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+struct l_cipher;
+
+enum l_cipher_type {
+	L_CIPHER_ARC4,
+	L_CIPHER_AES,
+};
+
+struct l_cipher *l_cipher_new(enum l_cipher_type type,
+				const void *key, size_t key_length);
+void l_cipher_free(struct l_cipher *cipher);
+
+void l_cipher_reset(struct l_cipher *cipher);
+
+void l_cipher_encrypt(struct l_cipher *cipher,
+				const void *data, size_t len);
+
+void l_cipher_decrypt(struct l_cipher *cipher,
+				const void *data, size_t len);
+
+void l_cipher_get_data(struct l_cipher *cipher,
+				void *data, size_t len);
+
+#endif /* __ELL_CIPHER_H */
-- 
2.0.5

Re: [PATCH] cipher: RFC on an API for a cipher utility infrastructure

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4143 bytes --]

Hi Tomasz,

On 01/28/2015 02:37 AM, Tomasz Bursztyka wrote:
> ---
>
> Hi guys,
>
> Here is a quick proposal on the cipher infra for ell. I followed what has
> been done with checksums in ell, with obvious differences due to the ciphers
> themselves. (2 different actions: encrypt/decrypt and the presence of a key)
>
> I have open issues here:
>
> l_cipher_reset() on its own might not be useful, should I add the possibilty
> to change the key as well? Unless there is no cipher state to reset, and then
> it's better not having such function. (I am not aware of the kernel cipher
> internals, related to some state there so if one can enlight the issue here)
>

I don't think a reset function would be useful.

> Alos, about encrypt/decrypt. Currently I am proposing to have those as they
> will send the data, and we get it back via get_data() (function name it not
> that great btw).
> So:
>
> - is having only encrypt/decrypt an option? (so send/recv would be done in
> each functions). Though 2 blocking functions at a time... not great.
>

I think that would be preferable.  I don't foresee us processing large 
chunks of encrypted data, so that should be okay.

> - or, what about enabling async access? Unless I miss something about the
> crypto layer in the kernel, here the socket is blocking. So we could be
> stalled at some point. checksum does the same, is there any plan to make it
> async actually? like a function l_cipher_enable_async(), which would call the
> right fcntl() and return the socket which we could use in the event loop.
>

Right now I wouldn't worry about this.  If we encounter a situation 
where we actually benefit from this, then lets revisit then.  Right now 
we are going to be encrypting/decrypting data on the order of a few 
hundred bytes.  EAPoL-Key Data is capped at about 32K, so even in the 
absolute worst case I don't think we need to worry about this.

>   ell/cipher.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
>   create mode 100644 ell/cipher.h
>
> diff --git a/ell/cipher.h b/ell/cipher.h
> new file mode 100644
> index 0000000..2a0ca2f
> --- /dev/null
> +++ b/ell/cipher.h
> @@ -0,0 +1,52 @@
> +/*
> + *
> + *  Embedded Linux library
> + *
> + *  Copyright (C) 2015  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifndef __ELL_CIPHER_H
> +#define __ELL_CIPHER_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct l_cipher;
> +
> +enum l_cipher_type {
> +	L_CIPHER_ARC4,
> +	L_CIPHER_AES,
> +};
> +
> +struct l_cipher *l_cipher_new(enum l_cipher_type type,
> +				const void *key, size_t key_length);
> +void l_cipher_free(struct l_cipher *cipher);
> +
> +void l_cipher_reset(struct l_cipher *cipher);
> +

No need for this one

> +void l_cipher_encrypt(struct l_cipher *cipher,
> +				const void *data, size_t len);

I'd just do l_cipher_encrypt(struct l_cipher *cipher,
				const void *in, void *out, size_t len);

> +
> +void l_cipher_decrypt(struct l_cipher *cipher,
> +				const void *data, size_t len);
> +

Same as above

> +void l_cipher_get_data(struct l_cipher *cipher,
> +				void *data, size_t len);
> +

No need for that one

> +#endif /* __ELL_CIPHER_H */
>

Regards,
-Denis

Re: [PATCH] cipher: RFC on an API for a cipher utility infrastructure

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]

Hi Marcel,

 >>
>> Right now I wouldn't worry about this.  If we encounter a situation where we actually benefit from this, then lets revisit then.  Right now we are going to be encrypting/decrypting data on the order of a few hundred bytes.  EAPoL-Key Data is capped at about 32K, so even in the absolute worst case I don't think we need to worry about this.
>
> Actually we might want to do this directly inside iwd instead of ELL. That background is pretty much that we need to run a basic selftest on available ciphers and algs anyway first. Not all kernels have the required ciphers (or even hash algorithm) that we need. So the advantage doing it in iwd direct without a framework in ELL is that we can quickly test the kernel and then just abort.
>

I tend to disagree.  We might as well make ell into a proper API.  If we 
need to enumerate supported AF_ALGs and add that functionality to ell, 
then that's what we have to do.

In the end, the fastest way to implement hashing and crypto is natively. 
  So if we decide to add that to ell at a later date, then we 
automatically make iwd faster without re-writing any code.

> It would also be an easier place to figure out what we really need in the end and what performance impact we are accepting by using the kernel crypto subsystem and AF_ALG.
>
> The checksum stuff has been added to ELL since we had that in GLib. However the HMAC part has never been added to ELL. That has been kept in iwd. And I am actually considering doing native AF_ALG for the checksum and HMAC without involving ELL. Reason is that we can be more efficient with the file descriptors and system calls if we target specific parts.
>

Yes, I thought that was pretty strange, but fixing it wasn't my priority 
right now.  In the end our target use case does not warrant any sort of 
'efficiency' tricks / optimizations, so I don't see the point. 
Especially given that the code is essentially the same in src/aes.c and 
ell/checksum.c

Regards,
-Denis

Re: [PATCH] cipher: RFC on an API for a cipher utility infrastructure

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 3142 bytes --]

Hi Tomasz,

>> Here is a quick proposal on the cipher infra for ell. I followed what has
>> been done with checksums in ell, with obvious differences due to the ciphers
>> themselves. (2 different actions: encrypt/decrypt and the presence of a key)
>> 
>> I have open issues here:
>> 
>> l_cipher_reset() on its own might not be useful, should I add the possibilty
>> to change the key as well? Unless there is no cipher state to reset, and then
>> it's better not having such function. (I am not aware of the kernel cipher
>> internals, related to some state there so if one can enlight the issue here)
>> 
> 
> I don't think a reset function would be useful.
> 
>> Alos, about encrypt/decrypt. Currently I am proposing to have those as they
>> will send the data, and we get it back via get_data() (function name it not
>> that great btw).
>> So:
>> 
>> - is having only encrypt/decrypt an option? (so send/recv would be done in
>> each functions). Though 2 blocking functions at a time... not great.
>> 
> 
> I think that would be preferable.  I don't foresee us processing large chunks of encrypted data, so that should be okay.

yes, this is the most we should do inside ELL. Anything more generic is not a good idea. You better self-code that in your project then. Using AF_ALG is always inefficient. Just for small amounts data it is irrelevant and using the kernel crypto subsystem keeps our code simpler.

>> - or, what about enabling async access? Unless I miss something about the
>> crypto layer in the kernel, here the socket is blocking. So we could be
>> stalled at some point. checksum does the same, is there any plan to make it
>> async actually? like a function l_cipher_enable_async(), which would call the
>> right fcntl() and return the socket which we could use in the event loop.
>> 
> 
> Right now I wouldn't worry about this.  If we encounter a situation where we actually benefit from this, then lets revisit then.  Right now we are going to be encrypting/decrypting data on the order of a few hundred bytes.  EAPoL-Key Data is capped at about 32K, so even in the absolute worst case I don't think we need to worry about this.

Actually we might want to do this directly inside iwd instead of ELL. That background is pretty much that we need to run a basic selftest on available ciphers and algs anyway first. Not all kernels have the required ciphers (or even hash algorithm) that we need. So the advantage doing it in iwd direct without a framework in ELL is that we can quickly test the kernel and then just abort.

It would also be an easier place to figure out what we really need in the end and what performance impact we are accepting by using the kernel crypto subsystem and AF_ALG.

The checksum stuff has been added to ELL since we had that in GLib. However the HMAC part has never been added to ELL. That has been kept in iwd. And I am actually considering doing native AF_ALG for the checksum and HMAC without involving ELL. Reason is that we can be more efficient with the file descriptors and system calls if we target specific parts.

Regards

Marcel