Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey
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
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(&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)
> - goto free_req;
>
> - crypto_ecdh_encode_key(buf, buf_len, &p);
> + 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(&p);
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + 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 */
> @@ -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(&result.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(&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_
[RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey
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(&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)
- goto free_req;
- crypto_ecdh_encode_key(buf, buf_len, &p);
+ 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(&p);
+ buf = kmalloc(buf_len, GFP_KERNEL);
+ if (!buf)
+ 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 */
@@ -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(&result.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(&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,
