Hi Tudor,
>>> That Bluetooth SMP knows about the private key is pointless, since the
>>> detection of debug key usage is actually via the public key portion.
>>> With this patch, the Bluetooth SMP will stop keeping a copy of the
>>> ecdh private key and will let the crypto subsystem to generate and
>>> handle the ecdh private key, potentially benefiting of hardware
>>> ecc private key generation and retention.
>>>
>>> The loop that tries to generate a correct private key is now removed and
>>> we trust the crypto subsystem to generate a correct private key. This
>>> backup logic should be done in crypto, if really needed.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
>>> ---
>>> net/bluetooth/ecdh_helper.c | 186 ++++++++++++++++++++++++--------------------
>>> net/bluetooth/ecdh_helper.h | 9 ++-
>>> net/bluetooth/selftest.c | 14 +++-
>>> net/bluetooth/smp.c | 66 +++++++---------
>>> 4 files changed, 147 insertions(+), 128 deletions(-)
>>>
>>> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
>>> index 16e022f..2155ce8 100644
>>> --- a/net/bluetooth/ecdh_helper.c
>>> +++ b/net/bluetooth/ecdh_helper.c
>>> @@ -49,15 +49,21 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned int ndigits)
>>> out[i] = __swab64(in[ndigits - 1 - i]);
>>> }
>>>
>>> +/* compute_ecdh_secret() - function assumes that the private key was
>>> + * already set.
>>> + * @tfm: KPP tfm handle allocated with crypto_alloc_kpp().
>>> + * @public_key: pair's ecc public key.
>>> + * secret: memory where the ecdh computed shared secret will be saved.
>>> + *
>>> + * Return: zero on success; error code in case of error.
>>> + */
>>> int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
>>> - const u8 private_key[32], u8 secret[32])
>>> + u8 secret[32])
>>> {
>>> struct kpp_request *req;
>>> - struct ecdh p;
>>> + u8 *tmp;
>>> struct ecdh_completion result;
>>> struct scatterlist src, dst;
>>> - u8 *tmp, *buf;
>>> - unsigned int buf_len;
>>> int err;
>>>
>>> tmp = kmalloc(64, GFP_KERNEL);
>>> @@ -72,28 +78,6 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
>>>
>>> init_completion(&result.completion);
>>>
>>> - /* Security Manager Protocol holds digits in litte-endian order
>>> - * while ECC API expect big-endian data
>>> - */
>>> - swap_digits((u64 *)private_key, (u64 *)tmp, 4);
>>> - p.key = (char *)tmp;
>>> - p.key_size = 32;
>>> - /* Set curve_id */
>>> - p.curve_id = ECC_CURVE_NIST_P256;
>>> - buf_len = crypto_ecdh_key_len(&p);
>>> - buf = kmalloc(buf_len, GFP_KERNEL);
>>> - if (!buf) {
>>> - err = -ENOMEM;
>>> - goto free_req;
>>> - }
>>> -
>>> - crypto_ecdh_encode_key(buf, buf_len, &p);
>>> -
>>> - /* Set A private Key */
>>> - err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
>>> - if (err)
>>> - goto free_all;
>>> -
>>> swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
>>> swap_digits((u64 *)&public_key[32], (u64 *)&tmp[32], 4); /* y */
>>>
>>> @@ -118,26 +102,76 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
>>> memcpy(secret, tmp, 32);
>>>
>>> free_all:
>>> - kzfree(buf);
>>> -free_req:
>>> kpp_request_free(req);
>>> free_tmp:
>>> kzfree(tmp);
>>> return err;
>>> }
>>>
>>> -int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
>>> - u8 private_key[32])
>>> +/* set_ecdh_privkey() - set or generate ecc private key.
>>> + *
>>> + * Function generates an ecc private key in the crypto subsystem when receiving
>>> + * a NULL private key or sets the received key when not NULL.
>>> + *
>>> + * @tfm: KPP tfm handle allocated with crypto_alloc_kpp().
>>> + * @private_key: user's ecc private key. When not NULL, the key is expected
>>> + * in little endian format.
>>> + *
>>> + * Return: zero on success; error code in case of error.
>>> + */
>>> +int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32])
>>> +{
>>> + u8 *buf, *tmp = NULL;
>>> + unsigned int buf_len;
>>> + int err;
>>> + struct ecdh p = {0};
>>> +
>>> + p.curve_id = ECC_CURVE_NIST_P256;
>>> +
>>> + if (private_key) {
>>> + tmp = kmalloc(32, GFP_KERNEL);
>>> + if (!tmp)
>>> + return -ENOMEM;
>>> + swap_digits((u64 *)private_key, (u64 *)tmp, 4);
>>> + p.key = tmp;
>>> + p.key_size = 32;
>>> + }
>>> +
>>> + buf_len = crypto_ecdh_key_len(&p);
>>> + buf = kmalloc(buf_len, GFP_KERNEL);
>>> + if (!buf) {
>>> + err = -ENOMEM;
>>> + goto free_tmp;
>>> + }
>>> +
>>> + err = crypto_ecdh_encode_key(buf, buf_len, &p);
>>> + if (err)
>>> + goto free_all;
>>> +
>>> + err = crypto_kpp_set_secret(tfm, buf, buf_len);
>>> + /* fall through */
>>> +free_all:
>>> + kzfree(buf);
>>> +free_tmp:
>>> + kzfree(tmp);
>>> + return err;
>>> +}
>>> +
>>> +/* generate_ecdh_public_key() - function assumes that the private key was
>>> + * already set.
>>> + *
>>> + * @tfm: KPP tfm handle allocated with crypto_alloc_kpp().
>>> + * @public_key: memory where the computed ecc public key will be saved.
>>> + *
>>> + * Return: zero on success; error code in case of error.
>>> + */
>>> +int generate_ecdh_public_key(struct crypto_kpp *tfm, u8 public_key[64])
>>> {
>>> struct kpp_request *req;
>>> - struct ecdh p;
>>> + u8 *tmp;
>>> struct ecdh_completion result;
>>> struct scatterlist dst;
>>> - u8 *tmp, *buf;
>>> - unsigned int buf_len;
>>> int err;
>>> - const unsigned short max_tries = 16;
>>> - unsigned short tries = 0;
>>>
>>> tmp = kmalloc(64, GFP_KERNEL);
>>> if (!tmp)
>>> @@ -150,63 +184,47 @@ int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
>>> }
>>>
>>> init_completion(&result.completion);
>>> + sg_init_one(&dst, tmp, 64);
>>> + kpp_request_set_input(req, NULL, 0);
>>> + kpp_request_set_output(req, &dst, 64);
>>> + kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>>> + ecdh_complete, &result);
>>>
>>> - /* Set curve_id */
>>> - p.curve_id = ECC_CURVE_NIST_P256;
>>> - p.key_size = 32;
>>> - buf_len = crypto_ecdh_key_len(&p);
>>> - buf = kmalloc(buf_len, GFP_KERNEL);
>>> - if (!buf)
>>> - goto free_req;
>>> -
>>> - do {
>>> - if (tries++ >= max_tries)
>>> - goto free_all;
>>> -
>>> - /* Set private Key */
>>> - p.key = (char *)private_key;
>>> - crypto_ecdh_encode_key(buf, buf_len, &p);
>>> - err = crypto_kpp_set_secret(tfm, buf, buf_len);
>>> - if (err)
>>> - goto free_all;
>>> -
>>> - sg_init_one(&dst, tmp, 64);
>>> - kpp_request_set_input(req, NULL, 0);
>>> - kpp_request_set_output(req, &dst, 64);
>>> - kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>>> - ecdh_complete, &result);
>>> -
>>> - err = crypto_kpp_generate_public_key(req);
>>> -
>>> - if (err == -EINPROGRESS) {
>>> - wait_for_completion(&result.completion);
>>> - err = result.err;
>>> - }
>>> -
>>> - /* Private key is not valid. Regenerate */
>>> - if (err == -EINVAL)
>>> - continue;
>>> -
>>> - if (err < 0)
>>> - goto free_all;
>>> - else
>>> - break;
>>> -
>>> - } while (true);
>>> -
>>> - /* Keys are handed back in little endian as expected by Security
>>> - * Manager Protocol
>>> + err = crypto_kpp_generate_public_key(req);
>>> + if (err == -EINPROGRESS) {
>>> + wait_for_completion(&result.completion);
>>> + err = result.err;
>>> + }
>>> + if (err < 0)
>>> + goto free_all;
>>> +
>>> + /* The public key is handed back in little endian as expected by
>>> + * the Security Manager Protocol.
>>> */
>>> swap_digits((u64 *)tmp, (u64 *)public_key, 4); /* x */
>>> swap_digits((u64 *)&tmp[32], (u64 *)&public_key[32], 4); /* y */
>>> - swap_digits((u64 *)private_key, (u64 *)tmp, 4);
>>> - memcpy(private_key, tmp, 32);
>>>
>>> free_all:
>>> - kzfree(buf);
>>> -free_req:
>>> kpp_request_free(req);
>>> free_tmp:
>>> kfree(tmp);
>>> return err;
>>> }
>>> +
>>> +/* generate_ecdh_keys() - generate ecc key pair.
>>> + *
>>> + * @tfm: KPP tfm handle allocated with crypto_alloc_kpp().
>>> + * @public_key: memory where the computed ecc public key will be saved.
>>> + *
>>> + * Return: zero on success; error code in case of error.
>>> + */
>>> +int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64])
>>> +{
>>> + int err;
>>> +
>>> + err = set_ecdh_privkey(tfm, NULL);
>>> + if (err)
>>> + return err;
>>> +
>>> + return generate_ecdh_public_key(tfm, public_key);
>>> +}
>>> diff --git a/net/bluetooth/ecdh_helper.h b/net/bluetooth/ecdh_helper.h
>>> index 50e6676..a6f8d03 100644
>>> --- a/net/bluetooth/ecdh_helper.h
>>> +++ b/net/bluetooth/ecdh_helper.h
>>> @@ -23,7 +23,8 @@
>>> #include <crypto/kpp.h>
>>> #include <linux/types.h>
>>>
>>> -int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pub_a[64],
>>> - const u8 priv_b[32], u8 secret[32]);
>>> -int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
>>> - u8 private_key[32]);
>>> +int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pair_public_key[64],
>>> + u8 secret[32]);
>>> +int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 *private_key);
>>> +int generate_ecdh_public_key(struct crypto_kpp *tfm, u8 public_key[64]);
>>> +int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64]);
>>> diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
>>> index ce99648..2d1519d 100644
>>> --- a/net/bluetooth/selftest.c
>>> +++ b/net/bluetooth/selftest.c
>>> @@ -152,11 +152,11 @@ static int __init test_ecdh_sample(struct crypto_kpp *tfm, const u8 priv_a[32],
>>> dhkey_a = &tmp[0];
>>> dhkey_b = &tmp[32];
>>>
>>> - ret = compute_ecdh_secret(tfm, pub_b, priv_a, dhkey_a);
>>> + ret = set_ecdh_privkey(tfm, priv_a);
>>> if (ret)
>>> goto out;
>>>
>>> - ret = compute_ecdh_secret(tfm, pub_a, priv_b, dhkey_b);
>>> + ret = compute_ecdh_secret(tfm, pub_b, dhkey_a);
>>> if (ret)
>>> goto out;
>>>
>>> @@ -165,9 +165,17 @@ static int __init test_ecdh_sample(struct crypto_kpp *tfm, const u8 priv_a[32],
>>> goto out;
>>> }
>>>
>>> + ret = set_ecdh_privkey(tfm, priv_b);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + ret = compute_ecdh_secret(tfm, pub_a, dhkey_b);
>>> + if (ret)
>>> + goto out;
>>> +
>>> if (memcmp(dhkey_b, dhkey, 32))
>>> ret = -EINVAL;
>>> -
>>> + /* fall through*/
>>> out:
>>> kfree(tmp);
>>> return ret;
>>> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
>>> index af7e610..d41449b 100644
>>> --- a/net/bluetooth/smp.c
>>> +++ b/net/bluetooth/smp.c
>>> @@ -84,7 +84,6 @@ enum {
>>> struct smp_dev {
>>> /* Secure Connections OOB data */
>>> u8 local_pk[64];
>>> - u8 local_sk[32];
>>> u8 local_rand[16];
>>> bool debug_key;
>>>
>>> @@ -126,7 +125,6 @@ struct smp_chan {
>>>
>>> /* Secure Connections variables */
>>> u8 local_pk[64];
>>> - u8 local_sk[32];
>>> u8 remote_pk[64];
>>> u8 dhkey[32];
>>> u8 mackey[16];
>>> @@ -568,24 +566,22 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
>>>
>>> if (hci_dev_test_flag(hdev, HCI_USE_DEBUG_KEYS)) {
>>> BT_DBG("Using debug keys");
>>> + err = set_ecdh_privkey(smp->tfm_ecdh, debug_sk);
>>> + if (err)
>>> + return err;
>>> memcpy(smp->local_pk, debug_pk, 64);
>>> - memcpy(smp->local_sk, debug_sk, 32);
>>> smp->debug_key = true;
>> this all looks good, but I do wonder why not have a helper function here.
>> err = generate_ecdh_debug_keys(smp->tfm_ecdh, smp->local_pk);
>> And then have that function defined like this:
>> int generate_ecdh_debug_keys(struct crypto_kpp *tfm, u8 public_key[64])
>> {
>> int err;
>> err = set_ecdh_privkey(tfm, debug_sk);
>> if (err)
>> return err;
>> memcpy(public_key, debug_pk, 64);
>> return 0;
>> }
>> I know this seems duplicate, but I wonder if that reduces the number of functions that have to be public. I prefer having most function static if possible.
>
> You want set_ecdh_privkey() to be static in ecdh_helper.c?
> test_ecdh_sample() and test_debug_key() don't need to copy the public
> key, so they need set_ecdh_privkey() as public.
>
> I can make generate_ecdh_debug_keys() static to smp.c, but we will be
> mixing helpers in smp.c and ecdh_helper.c.
would it make sense to just include the code from ecdh_helper.c in smp.c? I think that due to the usage of KPP it has shrunk a lot now. However we can do that in a follow up cleanup series. For now I am going to apply your patches.
Regards
Marcel
