[PATCH] crypto: af_alg - implement keyring support

[PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

This patch adds new socket options to AF_ALG that allow setting key from
kernel keyring. For simplicity, each keyring key type (logon, user,
trusted, encrypted) has its own socket option name and the value is just
the key description string that identifies the key to be used. The key
description doesn't need to be NULL-terminated, but bytes after the
first zero byte are ignored.

Note that this patch also adds three socket option names that are
already defined and used in libkcapi [1], but have never been added to
the kernel...

Tested via libkcapi with keyring patches [2] applied (user and logon key
types only).

[1] https://github.com/smuellerDD/libkcapi
[2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 crypto/af_alg.c             | 150 +++++++++++++++++++++++++++++++++++-
 include/uapi/linux/if_alg.h |   7 ++
 2 files changed, 156 insertions(+), 1 deletion(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index edca0998b2a4..fd6699e4dc3d 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -12,11 +12,15 @@
  *
  */
 
-#include <linux/atomic.h>
 #include <crypto/if_alg.h>
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <keys/encrypted-type.h>
+#include <linux/atomic.h>
 #include <linux/crypto.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/key.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/net.h>
@@ -230,6 +234,108 @@ out:
 	return err;
 }
 
+#ifdef CONFIG_KEYS
+struct alg_keyring_type {
+	struct key_type *key_type;
+	void (*key_from_payload)(void *payload, void **key, unsigned int *len);
+};
+
+static void alg_key_from_payload_user(void *payload, void **key,
+				      unsigned int *len)
+{
+	struct user_key_payload *ukp = payload;
+
+	*key = ukp->data;
+	*len = ukp->datalen;
+}
+
+static const struct alg_keyring_type alg_keyring_type_logon = {
+	.key_type = &key_type_logon,
+	.key_from_payload = &alg_key_from_payload_user,
+};
+static const struct alg_keyring_type alg_keyring_type_user = {
+	.key_type = &key_type_user,
+	.key_from_payload = &alg_key_from_payload_user,
+};
+
+#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
+static void alg_key_from_payload_trusted(void *payload, void **key,
+					 unsigned int *len)
+{
+	struct trusted_key_payload *tkp = payload;
+
+	*key = tkp->key;
+	*len = tkp->key_len;
+}
+
+static const struct alg_keyring_type alg_keyring_type_trusted = {
+	.key_type = &key_type_trusted,
+	.key_from_payload = &alg_key_from_payload_trusted,
+};
+#endif
+
+#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
+static void alg_key_from_payload_encrypted(void *payload, void **key,
+					   unsigned int *len)
+{
+	struct encrypted_key_payload *ekp = payload;
+
+	*key = ekp->decrypted_data;
+	*len = ekp->decrypted_datalen;
+}
+
+static const struct alg_keyring_type alg_keyring_type_encrypted = {
+	.key_type = &key_type_encrypted,
+	.key_from_payload = &alg_key_from_payload_encrypted,
+};
+#endif
+
+static int alg_setkey_keyring(struct sock *sk,
+			      const struct alg_keyring_type *type,
+			      char __user *udesc, unsigned int desclen)
+{
+	struct alg_sock *ask = alg_sk(sk);
+	struct key *key;
+	char *desc;
+	void *payload;
+	void *key_data;
+	unsigned int key_len;
+	int err;
+
+	desc = sock_kmalloc(sk, desclen + 1, GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	if (copy_from_user(desc, udesc, desclen)) {
+		sock_kzfree_s(sk, desc, desclen + 1);
+		return -EFAULT;
+	}
+	desc[desclen] = '\0';
+
+	key = request_key(type->key_type, desc, NULL);
+	sock_kzfree_s(sk, desc, desclen + 1);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	down_read(&key->sem);
+
+	payload = dereference_key_locked(key);
+	if (!payload) {
+		err = -EKEYREVOKED;
+		goto out;
+	}
+
+	type->key_from_payload(payload, &key_data, &key_len);
+
+	err = ask->type->setkey(ask->private, key_data, key_len);
+
+out:
+	up_read(&key->sem);
+	key_put(key);
+	return err;
+}
+#endif /* CONFIG_KEYS */
+
 static int alg_setsockopt(struct socket *sock, int level, int optname,
 			  char __user *optval, unsigned int optlen)
 {
@@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
 			goto unlock;
 
 		err = alg_setkey(sk, optval, optlen);
+#ifdef CONFIG_KEYS
+		break;
+	case ALG_SET_KEY_KEYRING_LOGON:
+		if (sock->state == SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
+					 optval, optlen);
+		break;
+	case ALG_SET_KEY_KEYRING_USER:
+		if (sock->state == SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_user,
+					 optval, optlen);
+#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
+		break;
+	case ALG_SET_KEY_KEYRING_TRUSTED:
+		if (sock->state == SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
+					 optval, optlen);
+#endif
+#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
+		break;
+	case ALG_SET_KEY_KEYRING_ENCRYPTED:
+		if (sock->state == SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
+					 optval, optlen);
+#endif
+#endif /* CONFIG_KEYS */
 		break;
 	case ALG_SET_AEAD_AUTHSIZE:
 		if (sock->state == SS_CONNECTED)
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..f2d777901f00 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,13 @@ struct af_alg_iv {
 #define ALG_SET_OP			3
 #define ALG_SET_AEAD_ASSOCLEN		4
 #define ALG_SET_AEAD_AUTHSIZE		5
+#define ALG_SET_PUBKEY			6 /* reserved for future use */
+#define ALG_SET_DH_PARAMETERS		7 /* reserved for future use */
+#define ALG_SET_ECDH_CURVE		8 /* reserved for future use */
+#define ALG_SET_KEY_KEYRING_LOGON	9
+#define ALG_SET_KEY_KEYRING_USER	10
+#define ALG_SET_KEY_KEYRING_TRUSTED	11
+#define ALG_SET_KEY_KEYRING_ENCRYPTED	12
 
 /* Operations */
 #define ALG_OP_DECRYPT			0
-- 
2.20.1

[PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

This patch adds new socket options to AF_ALG that allow setting key from
kernel keyring. For simplicity, each keyring key type (logon, user,
trusted, encrypted) has its own socket option name and the value is just
the key description string that identifies the key to be used. The key
description doesn't need to be NULL-terminated, but bytes after the
first zero byte are ignored.

Note that this patch also adds three socket option names that are
already defined and used in libkcapi [1], but have never been added to
the kernel...

Tested via libkcapi with keyring patches [2] applied (user and logon key
types only).

[1] https://github.com/smuellerDD/libkcapi
[2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 crypto/af_alg.c             | 150 +++++++++++++++++++++++++++++++++++-
 include/uapi/linux/if_alg.h |   7 ++
 2 files changed, 156 insertions(+), 1 deletion(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index edca0998b2a4..fd6699e4dc3d 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -12,11 +12,15 @@
  *
  */
 
-#include <linux/atomic.h>
 #include <crypto/if_alg.h>
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <keys/encrypted-type.h>
+#include <linux/atomic.h>
 #include <linux/crypto.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/key.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/net.h>
@@ -230,6 +234,108 @@ out:
 	return err;
 }
 
+#ifdef CONFIG_KEYS
+struct alg_keyring_type {
+	struct key_type *key_type;
+	void (*key_from_payload)(void *payload, void **key, unsigned int *len);
+};
+
+static void alg_key_from_payload_user(void *payload, void **key,
+				      unsigned int *len)
+{
+	struct user_key_payload *ukp = payload;
+
+	*key = ukp->data;
+	*len = ukp->datalen;
+}
+
+static const struct alg_keyring_type alg_keyring_type_logon = {
+	.key_type = &key_type_logon,
+	.key_from_payload = &alg_key_from_payload_user,
+};
+static const struct alg_keyring_type alg_keyring_type_user = {
+	.key_type = &key_type_user,
+	.key_from_payload = &alg_key_from_payload_user,
+};
+
+#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
+static void alg_key_from_payload_trusted(void *payload, void **key,
+					 unsigned int *len)
+{
+	struct trusted_key_payload *tkp = payload;
+
+	*key = tkp->key;
+	*len = tkp->key_len;
+}
+
+static const struct alg_keyring_type alg_keyring_type_trusted = {
+	.key_type = &key_type_trusted,
+	.key_from_payload = &alg_key_from_payload_trusted,
+};
+#endif
+
+#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
+static void alg_key_from_payload_encrypted(void *payload, void **key,
+					   unsigned int *len)
+{
+	struct encrypted_key_payload *ekp = payload;
+
+	*key = ekp->decrypted_data;
+	*len = ekp->decrypted_datalen;
+}
+
+static const struct alg_keyring_type alg_keyring_type_encrypted = {
+	.key_type = &key_type_encrypted,
+	.key_from_payload = &alg_key_from_payload_encrypted,
+};
+#endif
+
+static int alg_setkey_keyring(struct sock *sk,
+			      const struct alg_keyring_type *type,
+			      char __user *udesc, unsigned int desclen)
+{
+	struct alg_sock *ask = alg_sk(sk);
+	struct key *key;
+	char *desc;
+	void *payload;
+	void *key_data;
+	unsigned int key_len;
+	int err;
+
+	desc = sock_kmalloc(sk, desclen + 1, GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	if (copy_from_user(desc, udesc, desclen)) {
+		sock_kzfree_s(sk, desc, desclen + 1);
+		return -EFAULT;
+	}
+	desc[desclen] = '\0';
+
+	key = request_key(type->key_type, desc, NULL);
+	sock_kzfree_s(sk, desc, desclen + 1);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	down_read(&key->sem);
+
+	payload = dereference_key_locked(key);
+	if (!payload) {
+		err = -EKEYREVOKED;
+		goto out;
+	}
+
+	type->key_from_payload(payload, &key_data, &key_len);
+
+	err = ask->type->setkey(ask->private, key_data, key_len);
+
+out:
+	up_read(&key->sem);
+	key_put(key);
+	return err;
+}
+#endif /* CONFIG_KEYS */
+
 static int alg_setsockopt(struct socket *sock, int level, int optname,
 			  char __user *optval, unsigned int optlen)
 {
@@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
 			goto unlock;
 
 		err = alg_setkey(sk, optval, optlen);
+#ifdef CONFIG_KEYS
+		break;
+	case ALG_SET_KEY_KEYRING_LOGON:
+		if (sock->state = SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
+					 optval, optlen);
+		break;
+	case ALG_SET_KEY_KEYRING_USER:
+		if (sock->state = SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_user,
+					 optval, optlen);
+#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
+		break;
+	case ALG_SET_KEY_KEYRING_TRUSTED:
+		if (sock->state = SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
+					 optval, optlen);
+#endif
+#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
+		break;
+	case ALG_SET_KEY_KEYRING_ENCRYPTED:
+		if (sock->state = SS_CONNECTED)
+			goto unlock;
+		if (!type->setkey)
+			goto unlock;
+
+		err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
+					 optval, optlen);
+#endif
+#endif /* CONFIG_KEYS */
 		break;
 	case ALG_SET_AEAD_AUTHSIZE:
 		if (sock->state = SS_CONNECTED)
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..f2d777901f00 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,13 @@ struct af_alg_iv {
 #define ALG_SET_OP			3
 #define ALG_SET_AEAD_ASSOCLEN		4
 #define ALG_SET_AEAD_AUTHSIZE		5
+#define ALG_SET_PUBKEY			6 /* reserved for future use */
+#define ALG_SET_DH_PARAMETERS		7 /* reserved for future use */
+#define ALG_SET_ECDH_CURVE		8 /* reserved for future use */
+#define ALG_SET_KEY_KEYRING_LOGON	9
+#define ALG_SET_KEY_KEYRING_USER	10
+#define ALG_SET_KEY_KEYRING_TRUSTED	11
+#define ALG_SET_KEY_KEYRING_ENCRYPTED	12
 
 /* Operations */
 #define ALG_OP_DECRYPT			0
-- 
2.20.1

Re: [PATCH] crypto: af_alg - implement keyring support

[Marcel Holtmann]

Hi Ondrej,

> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.

why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.

Regards

Marcel

Re: [PATCH] crypto: af_alg - implement keyring support

[Marcel Holtmann]

Hi Ondrej,

> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.

why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.

Regards

Marcel

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

Hi Marcel,

On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Ondrej,
>
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
>
> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.

I was basing this on the approach taken by dm-crypt/cryptsetup, which
is actually the main target consumer for this feature (cryptsetup
needs to be able to encrypt/decrypt data using a keyring key (possibly
unreadable by userspace) without having to create a temporary dm-crypt
mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
just use key IDs there... @Milan/Ondrej, what was you motivation for
using key descriptions rather than key IDs?

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

Hi Marcel,

On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Ondrej,
>
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
>
> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.

I was basing this on the approach taken by dm-crypt/cryptsetup, which
is actually the main target consumer for this feature (cryptsetup
needs to be able to encrypt/decrypt data using a keyring key (possibly
unreadable by userspace) without having to create a temporary dm-crypt
mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
just use key IDs there... @Milan/Ondrej, what was you motivation for
using key descriptions rather than key IDs?

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Kozina]

On 5/21/19 1:02 PM, Ondrej Mosnacek wrote:
> Hi Marcel,
> 
> On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Ondrej,
>>
>>> This patch adds new socket options to AF_ALG that allow setting key from
>>> kernel keyring. For simplicity, each keyring key type (logon, user,
>>> trusted, encrypted) has its own socket option name and the value is just
>>> the key description string that identifies the key to be used. The key
>>> description doesn't need to be NULL-terminated, but bytes after the
>>> first zero byte are ignored.
>>
>> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
> 
> I was basing this on the approach taken by dm-crypt/cryptsetup, which
> is actually the main target consumer for this feature (cryptsetup
> needs to be able to encrypt/decrypt data using a keyring key (possibly
> unreadable by userspace) without having to create a temporary dm-crypt
> mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
> just use key IDs there... @Milan/Ondrej, what was you motivation for
> using key descriptions rather than key IDs?
> 
We decided to use key descriptions so that we could identify more 
clearly devices managed by cryptsetup (thus 'cryptsetup:' prefix in our 
descriptions put in dm-crypt table). I understood numeric ids were too 
generic for this purpose. Also cryptsetup uses by default thread keyring 
to upload keys in kernel. Such keys are obsolete the moment cryptsetup 
process finishes.

I don't think it's any problem to go with IDs for your patch. IIUC, as 
long as process has permission to access key metadata it can obtain also 
current key id so it's not a big problem for as to adapt when we use kcapi.

Regards
O.

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Kozina]

On 5/21/19 1:02 PM, Ondrej Mosnacek wrote:
> Hi Marcel,
> 
> On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Ondrej,
>>
>>> This patch adds new socket options to AF_ALG that allow setting key from
>>> kernel keyring. For simplicity, each keyring key type (logon, user,
>>> trusted, encrypted) has its own socket option name and the value is just
>>> the key description string that identifies the key to be used. The key
>>> description doesn't need to be NULL-terminated, but bytes after the
>>> first zero byte are ignored.
>>
>> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
> 
> I was basing this on the approach taken by dm-crypt/cryptsetup, which
> is actually the main target consumer for this feature (cryptsetup
> needs to be able to encrypt/decrypt data using a keyring key (possibly
> unreadable by userspace) without having to create a temporary dm-crypt
> mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
> just use key IDs there... @Milan/Ondrej, what was you motivation for
> using key descriptions rather than key IDs?
> 
We decided to use key descriptions so that we could identify more 
clearly devices managed by cryptsetup (thus 'cryptsetup:' prefix in our 
descriptions put in dm-crypt table). I understood numeric ids were too 
generic for this purpose. Also cryptsetup uses by default thread keyring 
to upload keys in kernel. Such keys are obsolete the moment cryptsetup 
process finishes.

I don't think it's any problem to go with IDs for your patch. IIUC, as 
long as process has permission to access key metadata it can obtain also 
current key id so it's not a big problem for as to adapt when we use kcapi.

Regards
O.

Re: [PATCH] crypto: af_alg - implement keyring support

[Stephan Müller]

Am Dienstag, 21. Mai 2019, 12:00:34 CEST schrieb Ondrej Mosnacek:

Hi Ondrej,

> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
> 
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
> 
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
> 
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Thank you!

Reviewed-by: Stephan Müller <smueller@chronox.de>

If the patch goes in, I will merge the libkcapi patch set and create a new 
release.

Ciao
Stephan

Re: [PATCH] crypto: af_alg - implement keyring support

[Stephan Müller]

Am Dienstag, 21. Mai 2019, 12:00:34 CEST schrieb Ondrej Mosnacek:

Hi Ondrej,

> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
> 
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
> 
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
> 
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Thank you!

Reviewed-by: Stephan Müller <smueller@chronox.de>

If the patch goes in, I will merge the libkcapi patch set and create a new 
release.

Ciao
Stephan

Re: [PATCH] crypto: af_alg - implement keyring support

[David Howells]

Marcel Holtmann <marcel@holtmann.org> wrote:

> why use the description instead the actual key id? I wonder if a single
> socket option and a struct providing the key type and key id might be more
> useful.

If the key becomes invalid in some way, you can call request_key() again if
you have the description to get a new key.  This is how afs and af_rxrpc do
things on the client side.

David

Re: [PATCH] crypto: af_alg - implement keyring support

[David Howells]

Marcel Holtmann <marcel@holtmann.org> wrote:

> why use the description instead the actual key id? I wonder if a single
> socket option and a struct providing the key type and key id might be more
> useful.

If the key becomes invalid in some way, you can call request_key() again if
you have the description to get a new key.  This is how afs and af_rxrpc do
things on the client side.

David

Re: [PATCH] crypto: af_alg - implement keyring support

[Eric Biggers]

On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
> 
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
> 
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
> 
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

The "interesting" thing about this is that given a key to which you have only
Search permission, you can request plaintext-ciphertext pairs with it using any
algorithm from the kernel's crypto API.  So if there are any really broken
algorithms and they happen to take the correct length key, you can effectively
read the key.  That's true even if you don't have Read permission on the key
and/or the key is of the "logon" type which doesn't have a ->read() method.

Since this is already also true for dm-crypt and maybe some other features in
the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
keys with Search access thus avoiding this problem and many others
(https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
this patch is applied.  But I wonder whether this is something you've actually
considered, and what security properties you think you are achieving by using
the Linux keyrings.

- Eric

Re: [PATCH] crypto: af_alg - implement keyring support

[Eric Biggers]

On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
> 
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
> 
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
> 
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

The "interesting" thing about this is that given a key to which you have only
Search permission, you can request plaintext-ciphertext pairs with it using any
algorithm from the kernel's crypto API.  So if there are any really broken
algorithms and they happen to take the correct length key, you can effectively
read the key.  That's true even if you don't have Read permission on the key
and/or the key is of the "logon" type which doesn't have a ->read() method.

Since this is already also true for dm-crypt and maybe some other features in
the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
keys with Search access thus avoiding this problem and many others
(https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
this patch is applied.  But I wonder whether this is something you've actually
considered, and what security properties you think you are achieving by using
the Linux keyrings.

- Eric

Re: [PATCH] crypto: af_alg - implement keyring support

[Milan Broz]

On 25/05/2019 05:10, Eric Biggers wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>> This patch adds new socket options to AF_ALG that allow setting key from
>> kernel keyring. For simplicity, each keyring key type (logon, user,
>> trusted, encrypted) has its own socket option name and the value is just
>> the key description string that identifies the key to be used. The key
>> description doesn't need to be NULL-terminated, but bytes after the
>> first zero byte are ignored.
>>
>> Note that this patch also adds three socket option names that are
>> already defined and used in libkcapi [1], but have never been added to
>> the kernel...
>>
>> Tested via libkcapi with keyring patches [2] applied (user and logon key
>> types only).
>>
>> [1] https://github.com/smuellerDD/libkcapi
>> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> 
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API.  So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key.  That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.

Yes, also if non-root user has access to some key in keyring, he can effectively
use it from another context (for example simulate dmcrypt and decrypt an image
of another user). But this is about policy of storing keys in keyrings.

> Since this is already also true for dm-crypt and maybe some other features in
> the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
> keys with Search access thus avoiding this problem and many others
> (https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
> this patch is applied.  But I wonder whether this is something you've actually
> considered, and what security properties you think you are achieving by using
> the Linux keyrings.

We use what kernel provides now. If there is a better way, we can switch to it.

The reason for using keyring for dmcrypt was to avoid sending key in plain
in every device-mapper ioctl structure. We use process keyring here, so
the key in keyring actually disappears after the setup utility finished its job.

The patch for crypto API is needed mainly for the "trusted" key type, that we
would like to support in dmcrypt as well. For integration in LUKS we need
to somehow validate that the key for particular dm-crypt device is valid.
If we cannot calculate a key digest directly (as LUKS does), we need to provide
some operation with the key from userspace (like HMAC) and compare known output.

This was the main reason for this patch. But I can imagine a lot of other
use cases where key in keyring actually simplifies things.

Milan

Re: [PATCH] crypto: af_alg - implement keyring support

[Milan Broz]

On 25/05/2019 05:10, Eric Biggers wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>> This patch adds new socket options to AF_ALG that allow setting key from
>> kernel keyring. For simplicity, each keyring key type (logon, user,
>> trusted, encrypted) has its own socket option name and the value is just
>> the key description string that identifies the key to be used. The key
>> description doesn't need to be NULL-terminated, but bytes after the
>> first zero byte are ignored.
>>
>> Note that this patch also adds three socket option names that are
>> already defined and used in libkcapi [1], but have never been added to
>> the kernel...
>>
>> Tested via libkcapi with keyring patches [2] applied (user and logon key
>> types only).
>>
>> [1] https://github.com/smuellerDD/libkcapi
>> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> 
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API.  So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key.  That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.

Yes, also if non-root user has access to some key in keyring, he can effectively
use it from another context (for example simulate dmcrypt and decrypt an image
of another user). But this is about policy of storing keys in keyrings.

> Since this is already also true for dm-crypt and maybe some other features in
> the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
> keys with Search access thus avoiding this problem and many others
> (https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
> this patch is applied.  But I wonder whether this is something you've actually
> considered, and what security properties you think you are achieving by using
> the Linux keyrings.

We use what kernel provides now. If there is a better way, we can switch to it.

The reason for using keyring for dmcrypt was to avoid sending key in plain
in every device-mapper ioctl structure. We use process keyring here, so
the key in keyring actually disappears after the setup utility finished its job.

The patch for crypto API is needed mainly for the "trusted" key type, that we
would like to support in dmcrypt as well. For integration in LUKS we need
to somehow validate that the key for particular dm-crypt device is valid.
If we cannot calculate a key digest directly (as LUKS does), we need to provide
some operation with the key from userspace (like HMAC) and compare known output.

This was the main reason for this patch. But I can imagine a lot of other
use cases where key in keyring actually simplifies things.

Milan

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

On Sat, May 25, 2019 at 5:10 AM Eric Biggers <ebiggers@kernel.org> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
> >
> > Note that this patch also adds three socket option names that are
> > already defined and used in libkcapi [1], but have never been added to
> > the kernel...
> >
> > Tested via libkcapi with keyring patches [2] applied (user and logon key
> > types only).
> >
> > [1] https://github.com/smuellerDD/libkcapi
> > [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API.  So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key.  That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.

Well, initially I was looking for a "KEY_NEED_USE" permission that
would allow using the key for encryption/decryption, but not to
actually read it. But then I was told by some people that the
KEY_NEED_SEARCH permission already serves exactly this purpose (i.e.
that when you can find the key, it means you can use it). I would
imagine that any practical use case for trusted keys would involve
encrypting/decrypting some data with the key (maybe not flowing
directly from/to userspace, but what use is a key with which you can
encrypt only some "internal" data...?), so I'm not sure where we want
to draw the boundary of what is safe to do with (userspace-unreadable
but findable) keyring keys... Maybe the keyring API needs some way to
control the intended usage of each key (something a bit like the "key
usage" in X.509 certificates [1]) - so you can e.g. mark some key to
be used for XYZ, but not for AF_ALG or dm-crypt...

Either way, I agree that this functionality opens up a potential
security hole (in that it makes it much more likely that a
vulnerability in the crypto drivers or crypto algorithms themselves
can reveal the value of a key that is not supposed to be readable by
userspace). However, I'm not sure how to mitigate this without some
new "KEY_NEED_PROCESS_ARBITRARY_DATA" permission or something... For
now I can at least add a Kconfig option to enable/disable keyring
support in AF_ALG so that people/distros who want both keyring and
AF_ALG enabled, but do not want to expose keyring keys via AF_ALG, can
just disable it.

BTW, I'm still undecided if I should convert this patch to use key IDs
rather than descriptions, but I tend to prefer to stay with the
current approach (mainly because it would be a lot of effort to
rewrite everything :)

[1] https://tools.ietf.org/html/rfc5280#section-4.2.1.3




--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

On Sat, May 25, 2019 at 5:10 AM Eric Biggers <ebiggers@kernel.org> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
> >
> > Note that this patch also adds three socket option names that are
> > already defined and used in libkcapi [1], but have never been added to
> > the kernel...
> >
> > Tested via libkcapi with keyring patches [2] applied (user and logon key
> > types only).
> >
> > [1] https://github.com/smuellerDD/libkcapi
> > [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API.  So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key.  That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.

Well, initially I was looking for a "KEY_NEED_USE" permission that
would allow using the key for encryption/decryption, but not to
actually read it. But then I was told by some people that the
KEY_NEED_SEARCH permission already serves exactly this purpose (i.e.
that when you can find the key, it means you can use it). I would
imagine that any practical use case for trusted keys would involve
encrypting/decrypting some data with the key (maybe not flowing
directly from/to userspace, but what use is a key with which you can
encrypt only some "internal" data...?), so I'm not sure where we want
to draw the boundary of what is safe to do with (userspace-unreadable
but findable) keyring keys... Maybe the keyring API needs some way to
control the intended usage of each key (something a bit like the "key
usage" in X.509 certificates [1]) - so you can e.g. mark some key to
be used for XYZ, but not for AF_ALG or dm-crypt...

Either way, I agree that this functionality opens up a potential
security hole (in that it makes it much more likely that a
vulnerability in the crypto drivers or crypto algorithms themselves
can reveal the value of a key that is not supposed to be readable by
userspace). However, I'm not sure how to mitigate this without some
new "KEY_NEED_PROCESS_ARBITRARY_DATA" permission or something... For
now I can at least add a Kconfig option to enable/disable keyring
support in AF_ALG so that people/distros who want both keyring and
AF_ALG enabled, but do not want to expose keyring keys via AF_ALG, can
just disable it.

BTW, I'm still undecided if I should convert this patch to use key IDs
rather than descriptions, but I tend to prefer to stay with the
current approach (mainly because it would be a lot of effort to
rewrite everything :)

[1] https://tools.ietf.org/html/rfc5280#section-4.2.1.3




--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] crypto: af_alg - implement keyring support

[Herbert Xu]

On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>
> @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
>  			goto unlock;
>  
>  		err = alg_setkey(sk, optval, optlen);
> +#ifdef CONFIG_KEYS
> +		break;
> +	case ALG_SET_KEY_KEYRING_LOGON:
> +		if (sock->state == SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> +					 optval, optlen);
> +		break;
> +	case ALG_SET_KEY_KEYRING_USER:
> +		if (sock->state == SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> +					 optval, optlen);
> +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> +		break;
> +	case ALG_SET_KEY_KEYRING_TRUSTED:
> +		if (sock->state == SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> +					 optval, optlen);
> +#endif
> +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> +		break;
> +	case ALG_SET_KEY_KEYRING_ENCRYPTED:
> +		if (sock->state == SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> +					 optval, optlen);
> +#endif
> +#endif /* CONFIG_KEYS */
>  		break;

What's with the funky placement of "break" outside of the ifdefs?

> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index bc2bcdec377b..f2d777901f00 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -35,6 +35,13 @@ struct af_alg_iv {
>  #define ALG_SET_OP			3
>  #define ALG_SET_AEAD_ASSOCLEN		4
>  #define ALG_SET_AEAD_AUTHSIZE		5
> +#define ALG_SET_PUBKEY			6 /* reserved for future use */
> +#define ALG_SET_DH_PARAMETERS		7 /* reserved for future use */
> +#define ALG_SET_ECDH_CURVE		8 /* reserved for future use */

Why do you need to reserve these values?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: af_alg - implement keyring support

[Herbert Xu]

On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>
> @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
>  			goto unlock;
>  
>  		err = alg_setkey(sk, optval, optlen);
> +#ifdef CONFIG_KEYS
> +		break;
> +	case ALG_SET_KEY_KEYRING_LOGON:
> +		if (sock->state = SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> +					 optval, optlen);
> +		break;
> +	case ALG_SET_KEY_KEYRING_USER:
> +		if (sock->state = SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> +					 optval, optlen);
> +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> +		break;
> +	case ALG_SET_KEY_KEYRING_TRUSTED:
> +		if (sock->state = SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> +					 optval, optlen);
> +#endif
> +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> +		break;
> +	case ALG_SET_KEY_KEYRING_ENCRYPTED:
> +		if (sock->state = SS_CONNECTED)
> +			goto unlock;
> +		if (!type->setkey)
> +			goto unlock;
> +
> +		err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> +					 optval, optlen);
> +#endif
> +#endif /* CONFIG_KEYS */
>  		break;

What's with the funky placement of "break" outside of the ifdefs?

> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index bc2bcdec377b..f2d777901f00 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -35,6 +35,13 @@ struct af_alg_iv {
>  #define ALG_SET_OP			3
>  #define ALG_SET_AEAD_ASSOCLEN		4
>  #define ALG_SET_AEAD_AUTHSIZE		5
> +#define ALG_SET_PUBKEY			6 /* reserved for future use */
> +#define ALG_SET_DH_PARAMETERS		7 /* reserved for future use */
> +#define ALG_SET_ECDH_CURVE		8 /* reserved for future use */

Why do you need to reserve these values?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: af_alg - implement keyring support

[Milan Broz]

Hi,

On 21/05/2019 12:00, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.

There is one nasty problem with this approach (we hit the same in dmcrypt now).

These lines cause hard module dependence on trusted.ko and encrypted.ko
modules (key_type_* are exported symbols):

 +static const struct alg_keyring_type alg_keyring_type_trusted = {
 +	.key_type = &key_type_trusted,...
...
 +	.key_type = &key_type_encrypted,

I do not think this is what we actually want - the dependence should be dynamic,
the modules should be loaded per-request...

I asked David Howells, and seems kernel keyring does not have such
interface yet. (There is an internal lookup function already though.)

So until such a lookup interface is present, and the patch is ported to it,
I think this patch adds module dependency that should not be there.

Milan

Re: [PATCH] crypto: af_alg - implement keyring support

[Milan Broz]

Hi,

On 21/05/2019 12:00, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.

There is one nasty problem with this approach (we hit the same in dmcrypt now).

These lines cause hard module dependence on trusted.ko and encrypted.ko
modules (key_type_* are exported symbols):

 +static const struct alg_keyring_type alg_keyring_type_trusted = {
 +	.key_type = &key_type_trusted,...
...
 +	.key_type = &key_type_encrypted,

I do not think this is what we actually want - the dependence should be dynamic,
the modules should be loaded per-request...

I asked David Howells, and seems kernel keyring does not have such
interface yet. (There is an internal lookup function already though.)

So until such a lookup interface is present, and the patch is ported to it,
I think this patch adds module dependency that should not be there.

Milan

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

On Thu, May 30, 2019 at 10:12 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> >
> > @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
> >                       goto unlock;
> >
> >               err = alg_setkey(sk, optval, optlen);
> > +#ifdef CONFIG_KEYS
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_LOGON:
> > +             if (sock->state == SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> > +                                      optval, optlen);
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_USER:
> > +             if (sock->state == SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> > +                                      optval, optlen);
> > +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_TRUSTED:
> > +             if (sock->state == SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> > +                                      optval, optlen);
> > +#endif
> > +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_ENCRYPTED:
> > +             if (sock->state == SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> > +                                      optval, optlen);
> > +#endif
> > +#endif /* CONFIG_KEYS */
> >               break;
>
> What's with the funky placement of "break" outside of the ifdefs?

I swear that checkpatch.pl was complaining when I did it the normal
way, but now I tried it again and it isn't complaining anymore...
Perhaps in the meantime I rebased onto a more recent tree where this
checkpatch.pl quirk has been fixed... I'll remove the funk in future
revisions.

>
> > diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> > index bc2bcdec377b..f2d777901f00 100644
> > --- a/include/uapi/linux/if_alg.h
> > +++ b/include/uapi/linux/if_alg.h
> > @@ -35,6 +35,13 @@ struct af_alg_iv {
> >  #define ALG_SET_OP                   3
> >  #define ALG_SET_AEAD_ASSOCLEN                4
> >  #define ALG_SET_AEAD_AUTHSIZE                5
> > +#define ALG_SET_PUBKEY                       6 /* reserved for future use */
> > +#define ALG_SET_DH_PARAMETERS                7 /* reserved for future use */
> > +#define ALG_SET_ECDH_CURVE           8 /* reserved for future use */
>
> Why do you need to reserve these values?

Because libkcapi already assumes these values [1] and has code that
uses them. Reserving will allow existing builds of libkcapi to work
correctly once the functionality actually lands in the kernel (if that
ever happens). Of course, it is libkcapi's fault to assume values for
these symbols (in released code) before they are commited in the
kernel, but it seemed easier to just add them along with this patch
rather than creating a confusing situation.

[1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek]

On Thu, May 30, 2019 at 10:12 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> >
> > @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
> >                       goto unlock;
> >
> >               err = alg_setkey(sk, optval, optlen);
> > +#ifdef CONFIG_KEYS
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_LOGON:
> > +             if (sock->state = SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> > +                                      optval, optlen);
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_USER:
> > +             if (sock->state = SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> > +                                      optval, optlen);
> > +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_TRUSTED:
> > +             if (sock->state = SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> > +                                      optval, optlen);
> > +#endif
> > +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> > +             break;
> > +     case ALG_SET_KEY_KEYRING_ENCRYPTED:
> > +             if (sock->state = SS_CONNECTED)
> > +                     goto unlock;
> > +             if (!type->setkey)
> > +                     goto unlock;
> > +
> > +             err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> > +                                      optval, optlen);
> > +#endif
> > +#endif /* CONFIG_KEYS */
> >               break;
>
> What's with the funky placement of "break" outside of the ifdefs?

I swear that checkpatch.pl was complaining when I did it the normal
way, but now I tried it again and it isn't complaining anymore...
Perhaps in the meantime I rebased onto a more recent tree where this
checkpatch.pl quirk has been fixed... I'll remove the funk in future
revisions.

>
> > diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> > index bc2bcdec377b..f2d777901f00 100644
> > --- a/include/uapi/linux/if_alg.h
> > +++ b/include/uapi/linux/if_alg.h
> > @@ -35,6 +35,13 @@ struct af_alg_iv {
> >  #define ALG_SET_OP                   3
> >  #define ALG_SET_AEAD_ASSOCLEN                4
> >  #define ALG_SET_AEAD_AUTHSIZE                5
> > +#define ALG_SET_PUBKEY                       6 /* reserved for future use */
> > +#define ALG_SET_DH_PARAMETERS                7 /* reserved for future use */
> > +#define ALG_SET_ECDH_CURVE           8 /* reserved for future use */
>
> Why do you need to reserve these values?

Because libkcapi already assumes these values [1] and has code that
uses them. Reserving will allow existing builds of libkcapi to work
correctly once the functionality actually lands in the kernel (if that
ever happens). Of course, it is libkcapi's fault to assume values for
these symbols (in released code) before they are commited in the
kernel, but it seemed easier to just add them along with this patch
rather than creating a confusing situation.

[1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] crypto: af_alg - implement keyring support

[Herbert Xu]

On Thu, May 30, 2019 at 01:35:06PM +0200, Ondrej Mosnacek wrote:
>
> Because libkcapi already assumes these values [1] and has code that
> uses them. Reserving will allow existing builds of libkcapi to work
> correctly once the functionality actually lands in the kernel (if that
> ever happens). Of course, it is libkcapi's fault to assume values for
> these symbols (in released code) before they are commited in the
> kernel, but it seemed easier to just add them along with this patch
> rather than creating a confusing situation.
> 
> [1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54

OK but please just leave a gap (or a comment) rather than actually
adding these reserved symbols to the kernel.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: af_alg - implement keyring support

[Herbert Xu]

On Thu, May 30, 2019 at 01:35:06PM +0200, Ondrej Mosnacek wrote:
>
> Because libkcapi already assumes these values [1] and has code that
> uses them. Reserving will allow existing builds of libkcapi to work
> correctly once the functionality actually lands in the kernel (if that
> ever happens). Of course, it is libkcapi's fault to assume values for
> these symbols (in released code) before they are commited in the
> kernel, but it seemed easier to just add them along with this patch
> rather than creating a confusing situation.
> 
> [1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54

OK but please just leave a gap (or a comment) rather than actually
adding these reserved symbols to the kernel.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt