diff mbox

[v3,7/7] crypto: AF_ALG - add support for key_id

Message ID 20160330005734.25410.28829.stgit@tstruk-mobl1 (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show

Commit Message

Tadeusz Struk March 30, 2016, 12:57 a.m. UTC 
This patch adds support for asymmetric key type to AF_ALG.
It will work as follows: A new PF_ALG socket options are
added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
ALG_SET_KEY_ID and ALG_SET_PUBKEY_ID for setting public and
private keys respectively. When these new options will be used
the user, instead of providing the key material, will provide a
key id and the key itself will be obtained from kernel keyring
subsystem. The user will use the standard tools (keyctl tool
or the keyctl syscall) for key instantiation and to obtain the
key id. The key id can also be obtained by reading the
/proc/keys file.

When a key is found, the request_key() function
returns the requested key. Next the asymmetric key subtype will be
used to obtain the public_key, which can be either a public key
or a private key. The key is then verified using the new
key info public_key_query_sw_key query, to check whether or not the key
can be accessed by software. In case when the key is kept
in hardware (TPM) then the function will return -ENOKEY error code.

If the key can be accesses by software then the key payload
will be passed to the akcipher pf_alg subtype.
AF_ALG code will then call crypto API functions, either the
crypto_akcipher_set_priv_key or the crypto_akcipher_set_pub_key,
depending on the used option. Subsequently the asymmetric key
will be freed and return code returned back to the user.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 crypto/af_alg.c             |   50 ++++++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/if_alg.h |    2 ++
 2 files changed, 48 insertions(+), 4 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

kernel test robot March 30, 2016, 1:49 a.m. UTC | #1 
Hi Tadeusz,

[auto build test ERROR on v4.6-rc1]
[also build test ERROR on next-20160329]
[cannot apply to crypto/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/crypto-algif-add-akcipher/20160330-090754
config: i386-randconfig-i1-03292045 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from crypto/af_alg.c:26:0:
   include/keys/asymmetric-type.h: In function 'asymmetric_key_ids':
>> include/keys/asymmetric-type.h:74:12: error: dereferencing pointer to incomplete type 'const struct key'
     return key->payload.data[asym_key_ids];
               ^
   crypto/af_alg.c: In function 'alg_setkey_id':
>> crypto/af_alg.c:217:12: error: implicit declaration of function 'request_key' [-Werror=implicit-function-declaration]
     keyring = request_key(&key_type_asymmetric, key_name, NULL);
               ^
>> crypto/af_alg.c:217:10: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     keyring = request_key(&key_type_asymmetric, key_name, NULL);
             ^
>> crypto/af_alg.c:223:16: error: dereferencing pointer to incomplete type 'struct key'
     pkey = keyring->payload.data[asym_crypto];
                   ^
   cc1: some warnings being treated as errors

vim +74 include/keys/asymmetric-type.h

7901c1a8 David Howells 2014-09-16  68  							    size_t len_1,
7901c1a8 David Howells 2014-09-16  69  							    const void *val_2,
7901c1a8 David Howells 2014-09-16  70  							    size_t len_2);
146aa8b1 David Howells 2015-10-21  71  static inline
146aa8b1 David Howells 2015-10-21  72  const struct asymmetric_key_ids *asymmetric_key_ids(const struct key *key)
146aa8b1 David Howells 2015-10-21  73  {
146aa8b1 David Howells 2015-10-21 @74  	return key->payload.data[asym_key_ids];
146aa8b1 David Howells 2015-10-21  75  }
7901c1a8 David Howells 2014-09-16  76  
7901c1a8 David Howells 2014-09-16  77  /*

:::::: The code at line 74 was first introduced by commit
:::::: 146aa8b1453bd8f1ff2304ffb71b4ee0eb9acdcc KEYS: Merge the type-specific data with the payload data

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: David Howells <dhowells@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot March 30, 2016, 2:22 a.m. UTC | #2 
Hi Tadeusz,

[auto build test ERROR on v4.6-rc1]
[also build test ERROR on next-20160329]
[cannot apply to crypto/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/crypto-algif-add-akcipher/20160330-090754
config: arm-at91_dt_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   In file included from crypto/af_alg.c:26:0:
   include/keys/asymmetric-type.h: In function 'asymmetric_key_ids':
>> include/keys/asymmetric-type.h:74:12: error: dereferencing pointer to incomplete type
     return key->payload.data[asym_key_ids];
               ^
   crypto/af_alg.c: In function 'alg_setkey_id':
   crypto/af_alg.c:217:2: error: implicit declaration of function 'request_key' [-Werror=implicit-function-declaration]
     keyring = request_key(&key_type_asymmetric, key_name, NULL);
     ^
   crypto/af_alg.c:217:10: warning: assignment makes pointer from integer without a cast
     keyring = request_key(&key_type_asymmetric, key_name, NULL);
             ^
>> crypto/af_alg.c:223:16: error: dereferencing pointer to incomplete type
     pkey = keyring->payload.data[asym_crypto];
                   ^
   cc1: some warnings being treated as errors

vim +74 include/keys/asymmetric-type.h

7901c1a8 David Howells 2014-09-16  68  							    size_t len_1,
7901c1a8 David Howells 2014-09-16  69  							    const void *val_2,
7901c1a8 David Howells 2014-09-16  70  							    size_t len_2);
146aa8b1 David Howells 2015-10-21  71  static inline
146aa8b1 David Howells 2015-10-21  72  const struct asymmetric_key_ids *asymmetric_key_ids(const struct key *key)
146aa8b1 David Howells 2015-10-21  73  {
146aa8b1 David Howells 2015-10-21 @74  	return key->payload.data[asym_key_ids];
146aa8b1 David Howells 2015-10-21  75  }
7901c1a8 David Howells 2014-09-16  76  
7901c1a8 David Howells 2014-09-16  77  /*

:::::: The code at line 74 was first introduced by commit
:::::: 146aa8b1453bd8f1ff2304ffb71b4ee0eb9acdcc KEYS: Merge the type-specific data with the payload data

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: David Howells <dhowells@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot March 30, 2016, 2:46 a.m. UTC | #3 
Hi Tadeusz,

[auto build test ERROR on v4.6-rc1]
[also build test ERROR on next-20160329]
[cannot apply to crypto/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/crypto-algif-add-akcipher/20160330-090754
config: x86_64-allyesdebian (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   crypto/built-in.o: In function `alg_setkey':
>> af_alg.c:(.text+0x36513): undefined reference to `key_type_asymmetric'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Tadeusz Struk March 30, 2016, 2:52 a.m. UTC | #4 
Hi,
On 03/29/2016 06:49 PM, kbuild test robot wrote:
> Hi Tadeusz,
> 
> [auto build test ERROR on v4.6-rc1]
> [also build test ERROR on next-20160329]
> [cannot apply to crypto/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/crypto-algif-add-akcipher/20160330-090754
> config: i386-randconfig-i1-03292045 (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):

The patches have been generated against cryptodev-2.6 rebased on top of 4.6-rc1.
It should work with crypto as soon as Herbert will rebase his repo.
Thanks,
David Howells March 30, 2016, 4:31 p.m. UTC | #5 
Tadeusz Struk <tadeusz.struk@intel.com> wrote:

> +	keyring = request_key(&key_type_asymmetric, key_name, NULL);
> +
> +	err = -ENOKEY;
> +	if (IS_ERR(keyring))
> +		goto out;
> +
> +	pkey = keyring->payload.data[asym_crypto];

NAK.  This is liable to crash in future.  You may not assume that you know
what keyring->payload.data[asym_crypto] points to.

You may not use struct public_key outside of crypto/asymmetric_key/.  It's the
internal data of the software subtype.  I'll move it out of the global header
to remove the temptation;-).

You must use accessor functions such as verify_signature().  Feel free to add
further accessor functions such as query_asym_capabilities(),
create_signature(), encrypt_blob() and decrypt_blob() or something like that.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Woodhouse March 30, 2016, 4:45 p.m. UTC | #6 
> Tadeusz Struk <tadeusz.struk@intel.com> wrote:
>
>> +	keyring = request_key(&key_type_asymmetric, key_name, NULL);
>> +
>> +	err = -ENOKEY;
>> +	if (IS_ERR(keyring))
>> +		goto out;
>> +
>> +	pkey = keyring->payload.data[asym_crypto];
>
> NAK.  This is liable to crash in future.  You may not assume that you know
> what keyring->payload.data[asym_crypto] points to.
>
> You may not use struct public_key outside of crypto/asymmetric_key/.  It's
> the
> internal data of the software subtype.  I'll move it out of the global
> header
> to remove the temptation;-).
>
> You must use accessor functions such as verify_signature().  Feel free to
> add
> further accessor functions such as query_asym_capabilities(),
> create_signature(), encrypt_blob() and decrypt_blob() or something like
> that.

Grr. This is not the first time this has been pointed out. And it
shouldn't have *needed* to be pointed out in the first place.

Please do not ignore review feedback.

Or common sense.
David Woodhouse March 30, 2016, 4:45 p.m. UTC | #7 
> Tadeusz Struk <tadeusz.struk@intel.com> wrote:
>
>> +	keyring = request_key(&key_type_asymmetric, key_name, NULL);
>> +
>> +	err = -ENOKEY;
>> +	if (IS_ERR(keyring))
>> +		goto out;
>> +
>> +	pkey = keyring->payload.data[asym_crypto];
>
> NAK.  This is liable to crash in future.  You may not assume that you know
> what keyring->payload.data[asym_crypto] points to.
>
> You may not use struct public_key outside of crypto/asymmetric_key/.  It's
> the
> internal data of the software subtype.  I'll move it out of the global
> header
> to remove the temptation;-).
>
> You must use accessor functions such as verify_signature().  Feel free to
> add
> further accessor functions such as query_asym_capabilities(),
> create_signature(), encrypt_blob() and decrypt_blob() or something like
> that.

Grr. This is not the first time this has been pointed out. And it
shouldn't have *needed* to be pointed out in the first place.

Please do not ignore review feedback.

Or common sense.
Tadeusz Struk March 30, 2016, 5:19 p.m. UTC | #8 
Hi David,
On 03/30/2016 09:31 AM, David Howells wrote:
>> +	keyring = request_key(&key_type_asymmetric, key_name, NULL);
>> > +
>> > +	err = -ENOKEY;
>> > +	if (IS_ERR(keyring))
>> > +		goto out;
>> > +
>> > +	pkey = keyring->payload.data[asym_crypto];
> NAK.  This is liable to crash in future.  You may not assume that you know
> what keyring->payload.data[asym_crypto] points to.
> 
> You may not use struct public_key outside of crypto/asymmetric_key/.  It's the
> internal data of the software subtype.  I'll move it out of the global header
> to remove the temptation;-).
> 
> You must use accessor functions such as verify_signature().  Feel free to add
> further accessor functions such as query_asym_capabilities(),
> create_signature(), encrypt_blob() and decrypt_blob() or something like that.

Thanks for your response. I thought that the public_key_query_sw_key(pkey)
check was enough for now.
I'll remove public_key stuff from af_alg and add the accessors.
Thanks,
diff mbox

Patch

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 24dc082..9d109bc 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -22,6 +22,8 @@ 
 #include <linux/net.h>
 #include <linux/rwsem.h>
 #include <linux/security.h>
+#include <crypto/public_key.h>
+#include <keys/asymmetric-type.h>
 
 struct alg_type_list {
 	const struct af_alg_type *type;
@@ -201,8 +203,40 @@  unlock:
 	return err;
 }
 
+static int alg_setkey_id(void *private, const u8 *key, unsigned int keylen,
+			 int (*setkey)(void *private, const u8 *key,
+				       unsigned int keylen))
+{
+	struct key *keyring;
+	struct public_key *pkey;
+	char key_name[12];
+	u32 keyid = *((u32 *)key);
+	int err;
+
+	sprintf(key_name, "id:%08x", keyid);
+	keyring = request_key(&key_type_asymmetric, key_name, NULL);
+
+	err = -ENOKEY;
+	if (IS_ERR(keyring))
+		goto out;
+
+	pkey = keyring->payload.data[asym_crypto];
+	if (!pkey)
+		goto out_put_key;
+
+	if (!public_key_query_sw_key(pkey))
+		goto out_put_key;
+
+	err = setkey(private, pkey->key, pkey->keylen);
+
+out_put_key:
+	key_put(keyring);
+out:
+	return err;
+}
+
 static int alg_setkey(struct sock *sk, char __user *ukey,
-		      unsigned int keylen,
+		      unsigned int keylen, bool key_id,
 		      int (*setkey)(void *private, const u8 *key,
 				    unsigned int keylen))
 {
@@ -221,7 +255,8 @@  static int alg_setkey(struct sock *sk, char __user *ukey,
 	if (copy_from_user(key, ukey, keylen))
 		goto out;
 
-	err = setkey(ask->private, key, keylen);
+	err = key_id ? alg_setkey_id(ask->private, key, keylen, setkey) :
+		       setkey(ask->private, key, keylen);
 
 out:
 	sock_kzfree_s(sk, key, keylen);
@@ -236,6 +271,8 @@  static int alg_setsockopt(struct socket *sock, int level, int optname,
 	struct alg_sock *ask = alg_sk(sk);
 	const struct af_alg_type *type;
 	int err = -EBUSY;
+	bool key_id = ((optname == ALG_SET_PUBKEY_ID) ||
+		       (optname == ALG_SET_KEY_ID));
 
 	lock_sock(sk);
 	if (ask->refcnt)
@@ -249,16 +286,21 @@  static int alg_setsockopt(struct socket *sock, int level, int optname,
 
 	switch (optname) {
 	case ALG_SET_KEY:
+	case ALG_SET_KEY_ID:
 		if (sock->state == SS_CONNECTED)
 			goto unlock;
+		/* ALG_SET_KEY_ID is only for akcipher */
+		if (!strcmp(type->name, "akcipher") && key_id)
+			goto unlock;
 
-		err = alg_setkey(sk, optval, optlen, type->setkey);
+		err = alg_setkey(sk, optval, optlen, key_id, type->setkey);
 		break;
 	case ALG_SET_PUBKEY:
+	case ALG_SET_PUBKEY_ID:
 		if (sock->state == SS_CONNECTED)
 			goto unlock;
 
-		err = alg_setkey(sk, optval, optlen, type->setpubkey);
+		err = alg_setkey(sk, optval, optlen, key_id, type->setpubkey);
 		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 02e6162..0379766 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,8 @@  struct af_alg_iv {
 #define ALG_SET_AEAD_ASSOCLEN		4
 #define ALG_SET_AEAD_AUTHSIZE		5
 #define ALG_SET_PUBKEY			6
+#define ALG_SET_PUBKEY_ID		7
+#define ALG_SET_KEY_ID			8
 
 /* Operations */
 #define ALG_OP_DECRYPT			0