[PATCH] Bluetooth: allocate data for kpp on heap

[PATCH] Bluetooth: allocate data for kpp on heap

[Salvatore Benedetto]

Bluetooth would crash when computing ECDH keys with kpp
if VMAP_STACK is enabled. Fix by allocating data passed
to kpp on heap.

Fixes: 58771c1c ("Bluetooth: convert smp and selftest to crypto kpp
API")
Signed-off-by: Salvatore Benedetto <salvatore.benedetto@intel.com>
---
 net/bluetooth/ecdh_helper.c |  6 ++++--
 net/bluetooth/selftest.c    | 16 +++++++++++-----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index b6d9aa1..8018447 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -59,7 +59,7 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8 private_key[32],
 	struct ecdh p;
 	struct ecdh_completion result;
 	struct scatterlist src, dst;
-	u8 tmp[64];
+	u8 *tmp = kmalloc(64, GFP_KERNEL);
 	u8 *buf;
 	unsigned int buf_len;
 	int err = -ENOMEM;
@@ -68,7 +68,7 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8 private_key[32],
 	if (IS_ERR(tfm)) {
 		pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
 		       PTR_ERR(tfm));
-		return false;
+		goto free_tmp;
 	}
 
 	req = kpp_request_alloc(tfm, GFP_KERNEL);
@@ -128,6 +128,8 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8 private_key[32],
 	kpp_request_free(req);
 free_kpp:
 	crypto_free_kpp(tfm);
+free_tmp:
+	kfree(tmp);
 	return (err == 0);
 }
 
diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
index efef281..9a72f8f 100644
--- a/net/bluetooth/selftest.c
+++ b/net/bluetooth/selftest.c
@@ -142,18 +142,24 @@ static int __init test_ecdh_sample(const u8 priv_a[32], const u8 priv_b[32],
 				   const u8 pub_a[64], const u8 pub_b[64],
 				   const u8 dhkey[32])
 {
-	u8 dhkey_a[32], dhkey_b[32];
+	u8 *dhkey_a = kmalloc(64, GFP_KERNEL);
+	u8 *dhkey_b = &dhkey_a[32];
+	int ret = 0;
 
 	compute_ecdh_secret(pub_b, priv_a, dhkey_a);
 	compute_ecdh_secret(pub_a, priv_b, dhkey_b);
 
-	if (memcmp(dhkey_a, dhkey, 32))
-		return -EINVAL;
+	if (memcmp(dhkey_a, dhkey, 32)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	if (memcmp(dhkey_b, dhkey, 32))
-		return -EINVAL;
+		ret = -EINVAL;
 
-	return 0;
+out:
+	kfree(dhkey_a);
+	return ret;
 }
 
 static char test_ecdh_buffer[32];
-- 
2.7.4

Re: [PATCH] Bluetooth: allocate data for kpp on heap

[Marcel Holtmann]

Hi Salvatore,

> Bluetooth would crash when computing ECDH keys with kpp
> if VMAP_STACK is enabled. Fix by allocating data passed
> to kpp on heap.
> 
> Fixes: 58771c1c ("Bluetooth: convert smp and selftest to crypto kpp
> API")
> Signed-off-by: Salvatore Benedetto <salvatore.benedetto@intel.com>
> ---
> net/bluetooth/ecdh_helper.c |  6 ++++--
> net/bluetooth/selftest.c    | 16 +++++++++++-----
> 2 files changed, 15 insertions(+), 7 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH] Bluetooth: allocate data for kpp on heap

[Szymon Janc]

Hi,

On Tuesday, 25 April 2017 08:59:47 PDT Salvatore Benedetto wrote:
> Bluetooth would crash when computing ECDH keys with kpp
> if VMAP_STACK is enabled. Fix by allocating data passed
> to kpp on heap.
> 
> Fixes: 58771c1c ("Bluetooth: convert smp and selftest to crypto kpp
> API")
> Signed-off-by: Salvatore Benedetto <salvatore.benedetto@intel.com>
> ---
>  net/bluetooth/ecdh_helper.c |  6 ++++--
>  net/bluetooth/selftest.c    | 16 +++++++++++-----
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index b6d9aa1..8018447 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -59,7 +59,7 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8
> private_key[32], struct ecdh p;
>  	struct ecdh_completion result;
>  	struct scatterlist src, dst;
> -	u8 tmp[64];
> +	u8 *tmp = kmalloc(64, GFP_KERNEL);

Should this be checked for null?

>  	u8 *buf;
>  	unsigned int buf_len;
>  	int err = -ENOMEM;
> @@ -68,7 +68,7 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8
> private_key[32], if (IS_ERR(tfm)) {
>  		pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
>  		       PTR_ERR(tfm));
> -		return false;
> +		goto free_tmp;
>  	}
> 
>  	req = kpp_request_alloc(tfm, GFP_KERNEL);
> @@ -128,6 +128,8 @@ bool compute_ecdh_secret(const u8 public_key[64], const
> u8 private_key[32], kpp_request_free(req);
>  free_kpp:
>  	crypto_free_kpp(tfm);
> +free_tmp:
> +	kfree(tmp);
>  	return (err == 0);
>  }
> 
> diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
> index efef281..9a72f8f 100644
> --- a/net/bluetooth/selftest.c
> +++ b/net/bluetooth/selftest.c
> @@ -142,18 +142,24 @@ static int __init test_ecdh_sample(const u8
> priv_a[32], const u8 priv_b[32], const u8 pub_a[64], const u8 pub_b[64],
>  				   const u8 dhkey[32])
>  {
> -	u8 dhkey_a[32], dhkey_b[32];
> +	u8 *dhkey_a = kmalloc(64, GFP_KERNEL);
> +	u8 *dhkey_b = &dhkey_a[32];
> +	int ret = 0;
> 
>  	compute_ecdh_secret(pub_b, priv_a, dhkey_a);
>  	compute_ecdh_secret(pub_a, priv_b, dhkey_b);
> 
> -	if (memcmp(dhkey_a, dhkey, 32))
> -		return -EINVAL;
> +	if (memcmp(dhkey_a, dhkey, 32)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> 
>  	if (memcmp(dhkey_b, dhkey, 32))
> -		return -EINVAL;
> +		ret = -EINVAL;
> 
> -	return 0;
> +out:
> +	kfree(dhkey_a);
> +	return ret;
>  }
> 
>  static char test_ecdh_buffer[32];


-- 
pozdrawiam
Szymon Janc

Re: [PATCH] Bluetooth: allocate data for kpp on heap

[Marcel Holtmann]

Hi Szymon,

>> Bluetooth would crash when computing ECDH keys with kpp
>> if VMAP_STACK is enabled. Fix by allocating data passed
>> to kpp on heap.
>> 
>> Fixes: 58771c1c ("Bluetooth: convert smp and selftest to crypto kpp
>> API")
>> Signed-off-by: Salvatore Benedetto <salvatore.benedetto@intel.com>
>> ---
>> net/bluetooth/ecdh_helper.c |  6 ++++--
>> net/bluetooth/selftest.c    | 16 +++++++++++-----
>> 2 files changed, 15 insertions(+), 7 deletions(-)
>> 
>> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
>> index b6d9aa1..8018447 100644
>> --- a/net/bluetooth/ecdh_helper.c
>> +++ b/net/bluetooth/ecdh_helper.c
>> @@ -59,7 +59,7 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8
>> private_key[32], struct ecdh p;
>> 	struct ecdh_completion result;
>> 	struct scatterlist src, dst;
>> -	u8 tmp[64];
>> +	u8 *tmp = kmalloc(64, GFP_KERNEL);
> 
> Should this be checked for null?

possible yes, how do other crypto users did this when they converted to VMAP_STACK support. We should do exactly the same as others did.

Regards

Marcel