Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

Hi, Marcel,

Agreed on all suggestions, I will send a v2 patch set.

Thanks,
ta

Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

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, except when using debug keys. This way we 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.
> 
> Keeping the private key in the crypto subsystem implies that we can't
> check if we accidentally generated a debug key. As this event is
> unlikely we can live with it when comparing with the benefit of the
> overall change: hardware private key generation and retention.
> 
> Signed-off-by: Tudor Ambarus 
> ---
> net/bluetooth/ecdh_helper.c | 102 +---
> net/bluetooth/smp.c |  55 +---
> 2 files changed, 67 insertions(+), 90 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index ac2c708..56e9374 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -53,10 +53,10 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
> public_key[64],
>const u8 private_key[32], u8 secret[32])
> {
>   struct kpp_request *req;
> - struct ecdh p;
> + struct ecdh p = {0};
>   struct ecdh_completion result;
>   struct scatterlist src, dst;
> - u8 *tmp, *buf;
> + u8 *tmp, *buf = NULL;
>   unsigned int buf_len;
>   int err = -ENOMEM;
> 
> @@ -70,25 +70,29 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
> public_key[64],
> 
>   init_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();
> - buf = kmalloc(buf_len, GFP_KERNEL);
> - if (!buf)
> - goto free_req;
> 
> - crypto_ecdh_encode_key(buf, buf_len, );
> + if (private_key) {
> + /* Security Manager Protocol holds digits in little-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 A private Key */
> - err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
> - if (err)
> - goto free_all;
> + buf_len = crypto_ecdh_key_len();
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + goto free_req;
> +
> + crypto_ecdh_encode_key(buf, buf_len, );
> +
> + /* 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 *)_key[32], (u64 *)[32], 4); /* y */
> @@ -126,14 +130,12 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
> public_key[64],
>   u8 private_key[32])
> {
>   struct kpp_request *req;
> - struct ecdh p;
> + struct ecdh p = {0};
>   struct ecdh_completion result;
>   struct scatterlist dst;
>   u8 *tmp, *buf;
>   unsigned int buf_len;
>   int err = -ENOMEM;
> - const unsigned short max_tries = 16;
> - unsigned short tries = 0;
> 
>   tmp = kmalloc(64, GFP_KERNEL);
>   if (!tmp)
> @@ -145,56 +147,48 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
> public_key[64],
> 
>   init_completion();
> 
> + /* Set private Key */
> + if (private_key) {
> + p.key = (char *)private_key;
> + p.key_size = 32;
> + }
> +
>   /* Set curve_id */
>   p.curve_id = ECC_CURVE_NIST_P256;
> - p.key_size = 32;
>   buf_len = crypto_ecdh_key_len();
>   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, );
> - err = crypto_kpp_set_secret(tfm, buf, buf_len);
> - if (err)
> - goto free_all;
> -
> - sg_init_one(, tmp, 64);
> - kpp_request_set_input(req, NULL, 0);
> - kpp_request_set_output(req, , 64);
> -