[v2] crypto/ecc: Actually remove stack VLA usage
diff mbox

Message ID 20180330165544.GA13206@beast
State Accepted
Delegated to: Herbert Xu
Headers show

Commit Message

Kees Cook March 30, 2018, 4:55 p.m. UTC
On the quest to remove all VLAs from the kernel[1], this avoids VLAs
by just using the maximum allocation size (4 bytes) for stack arrays.
All the VLAs in ecc were either 3 or 4 bytes (or a multiple), so just
make it 4 bytes all the time. Initialization routines are adjusted to
check that ndigits does not end up larger than the arrays.

This includes a removal of the earlier attempt at this fix from
commit a963834b4742 ("crypto/ecc: Remove stack VLA usage")

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
- Squash revert (herbert)
---
 crypto/ecc.c  | 66 +++++++++++++++++++++++++----------------------------------
 crypto/ecc.h  |  4 +++-
 crypto/ecdh.c |  4 ++--
 3 files changed, 33 insertions(+), 41 deletions(-)

Comments

Kees Cook April 16, 2018, 4:04 p.m. UTC | #1
On Fri, Mar 30, 2018 at 9:55 AM, Kees Cook <keescook@chromium.org> wrote:
> On the quest to remove all VLAs from the kernel[1], this avoids VLAs
> by just using the maximum allocation size (4 bytes) for stack arrays.
> All the VLAs in ecc were either 3 or 4 bytes (or a multiple), so just
> make it 4 bytes all the time. Initialization routines are adjusted to
> check that ndigits does not end up larger than the arrays.
>
> This includes a removal of the earlier attempt at this fix from
> commit a963834b4742 ("crypto/ecc: Remove stack VLA usage")
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Friendly ping. Please apply to fix a963834b4742 ("crypto/ecc: Remove
stack VLA usage").

Thanks!

-Kees

> ---
> v2:
> - Squash revert (herbert)
> ---
>  crypto/ecc.c  | 66 +++++++++++++++++++++++++----------------------------------
>  crypto/ecc.h  |  4 +++-
>  crypto/ecdh.c |  4 ++--
>  3 files changed, 33 insertions(+), 41 deletions(-)
>
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 9c066b5ac12d..815541309a95 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -515,7 +515,7 @@ static void vli_mmod_fast_256(u64 *result, const u64 *product,
>  static bool vli_mmod_fast(u64 *result, u64 *product,
>                           const u64 *curve_prime, unsigned int ndigits)
>  {
> -       u64 tmp[2 * ndigits];
> +       u64 tmp[2 * ECC_MAX_DIGITS];
>
>         switch (ndigits) {
>         case 3:
> @@ -536,7 +536,7 @@ static bool vli_mmod_fast(u64 *result, u64 *product,
>  static void vli_mod_mult_fast(u64 *result, const u64 *left, const u64 *right,
>                               const u64 *curve_prime, unsigned int ndigits)
>  {
> -       u64 product[2 * ndigits];
> +       u64 product[2 * ECC_MAX_DIGITS];
>
>         vli_mult(product, left, right, ndigits);
>         vli_mmod_fast(result, product, curve_prime, ndigits);
> @@ -546,7 +546,7 @@ static void vli_mod_mult_fast(u64 *result, const u64 *left, const u64 *right,
>  static void vli_mod_square_fast(u64 *result, const u64 *left,
>                                 const u64 *curve_prime, unsigned int ndigits)
>  {
> -       u64 product[2 * ndigits];
> +       u64 product[2 * ECC_MAX_DIGITS];
>
>         vli_square(product, left, ndigits);
>         vli_mmod_fast(result, product, curve_prime, ndigits);
> @@ -560,8 +560,8 @@ static void vli_mod_square_fast(u64 *result, const u64 *left,
>  static void vli_mod_inv(u64 *result, const u64 *input, const u64 *mod,
>                         unsigned int ndigits)
>  {
> -       u64 a[ndigits], b[ndigits];
> -       u64 u[ndigits], v[ndigits];
> +       u64 a[ECC_MAX_DIGITS], b[ECC_MAX_DIGITS];
> +       u64 u[ECC_MAX_DIGITS], v[ECC_MAX_DIGITS];
>         u64 carry;
>         int cmp_result;
>
> @@ -649,8 +649,8 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 *z1,
>                                       u64 *curve_prime, unsigned int ndigits)
>  {
>         /* t1 = x, t2 = y, t3 = z */
> -       u64 t4[ndigits];
> -       u64 t5[ndigits];
> +       u64 t4[ECC_MAX_DIGITS];
> +       u64 t5[ECC_MAX_DIGITS];
>
>         if (vli_is_zero(z1, ndigits))
>                 return;
> @@ -711,7 +711,7 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 *z1,
>  static void apply_z(u64 *x1, u64 *y1, u64 *z, u64 *curve_prime,
>                     unsigned int ndigits)
>  {
> -       u64 t1[ndigits];
> +       u64 t1[ECC_MAX_DIGITS];
>
>         vli_mod_square_fast(t1, z, curve_prime, ndigits);    /* z^2 */
>         vli_mod_mult_fast(x1, x1, t1, curve_prime, ndigits); /* x1 * z^2 */
> @@ -724,7 +724,7 @@ static void xycz_initial_double(u64 *x1, u64 *y1, u64 *x2, u64 *y2,
>                                 u64 *p_initial_z, u64 *curve_prime,
>                                 unsigned int ndigits)
>  {
> -       u64 z[ndigits];
> +       u64 z[ECC_MAX_DIGITS];
>
>         vli_set(x2, x1, ndigits);
>         vli_set(y2, y1, ndigits);
> @@ -750,7 +750,7 @@ static void xycz_add(u64 *x1, u64 *y1, u64 *x2, u64 *y2, u64 *curve_prime,
>                      unsigned int ndigits)
>  {
>         /* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
> -       u64 t5[ndigits];
> +       u64 t5[ECC_MAX_DIGITS];
>
>         /* t5 = x2 - x1 */
>         vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
> @@ -791,9 +791,9 @@ static void xycz_add_c(u64 *x1, u64 *y1, u64 *x2, u64 *y2, u64 *curve_prime,
>                        unsigned int ndigits)
>  {
>         /* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
> -       u64 t5[ndigits];
> -       u64 t6[ndigits];
> -       u64 t7[ndigits];
> +       u64 t5[ECC_MAX_DIGITS];
> +       u64 t6[ECC_MAX_DIGITS];
> +       u64 t7[ECC_MAX_DIGITS];
>
>         /* t5 = x2 - x1 */
>         vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
> @@ -846,9 +846,9 @@ static void ecc_point_mult(struct ecc_point *result,
>                            unsigned int ndigits)
>  {
>         /* R0 and R1 */
> -       u64 rx[2][ndigits];
> -       u64 ry[2][ndigits];
> -       u64 z[ndigits];
> +       u64 rx[2][ECC_MAX_DIGITS];
> +       u64 ry[2][ECC_MAX_DIGITS];
> +       u64 z[ECC_MAX_DIGITS];
>         int i, nb;
>         int num_bits = vli_num_bits(scalar, ndigits);
>
> @@ -943,13 +943,13 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
>  int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
>  {
>         const struct ecc_curve *curve = ecc_get_curve(curve_id);
> -       u64 priv[ndigits];
> +       u64 priv[ECC_MAX_DIGITS];
>         unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
>         unsigned int nbits = vli_num_bits(curve->n, ndigits);
>         int err;
>
>         /* Check that N is included in Table 1 of FIPS 186-4, section 6.1.1 */
> -       if (nbits < 160)
> +       if (nbits < 160 || ndigits > ARRAY_SIZE(priv))
>                 return -EINVAL;
>
>         /*
> @@ -988,10 +988,10 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
>  {
>         int ret = 0;
>         struct ecc_point *pk;
> -       u64 priv[ndigits];
> +       u64 priv[ECC_MAX_DIGITS];
>         const struct ecc_curve *curve = ecc_get_curve(curve_id);
>
> -       if (!private_key || !curve) {
> +       if (!private_key || !curve || ndigits > ARRAY_SIZE(priv)) {
>                 ret = -EINVAL;
>                 goto out;
>         }
> @@ -1025,30 +1025,25 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>  {
>         int ret = 0;
>         struct ecc_point *product, *pk;
> -       u64 *priv, *rand_z;
> +       u64 priv[ECC_MAX_DIGITS];
> +       u64 rand_z[ECC_MAX_DIGITS];
> +       unsigned int nbytes;
>         const struct ecc_curve *curve = ecc_get_curve(curve_id);
>
> -       if (!private_key || !public_key || !curve) {
> +       if (!private_key || !public_key || !curve ||
> +           ndigits > ARRAY_SIZE(priv) || ndigits > ARRAY_SIZE(rand_z)) {
>                 ret = -EINVAL;
>                 goto out;
>         }
>
> -       priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
> -       if (!priv) {
> -               ret = -ENOMEM;
> -               goto out;
> -       }
> +       nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
>
> -       rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
> -       if (!rand_z) {
> -               ret = -ENOMEM;
> -               goto kfree_out;
> -       }
> +       get_random_bytes(rand_z, nbytes);
>
>         pk = ecc_alloc_point(ndigits);
>         if (!pk) {
>                 ret = -ENOMEM;
> -               goto kfree_out;
> +               goto out;
>         }
>
>         product = ecc_alloc_point(ndigits);
> @@ -1057,8 +1052,6 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>                 goto err_alloc_product;
>         }
>
> -       get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
> -
>         ecc_swap_digits(public_key, pk->x, ndigits);
>         ecc_swap_digits(&public_key[ndigits], pk->y, ndigits);
>         ecc_swap_digits(private_key, priv, ndigits);
> @@ -1073,9 +1066,6 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
>         ecc_free_point(product);
>  err_alloc_product:
>         ecc_free_point(pk);
> -kfree_out:
> -       kzfree(priv);
> -       kzfree(rand_z);
>  out:
>         return ret;
>  }
> diff --git a/crypto/ecc.h b/crypto/ecc.h
> index e4fd4492c765..f75a86baa3bd 100644
> --- a/crypto/ecc.h
> +++ b/crypto/ecc.h
> @@ -26,7 +26,9 @@
>  #ifndef _CRYPTO_ECC_H
>  #define _CRYPTO_ECC_H
>
> -#define ECC_MAX_DIGITS 4 /* 256 */
> +#define ECC_CURVE_NIST_P192_DIGITS  3
> +#define ECC_CURVE_NIST_P256_DIGITS  4
> +#define ECC_MAX_DIGITS              ECC_CURVE_NIST_P256_DIGITS
>
>  #define ECC_DIGITS_TO_BYTES_SHIFT 3
>
> diff --git a/crypto/ecdh.c b/crypto/ecdh.c
> index 3aca0933ec44..3f91ef13c8c6 100644
> --- a/crypto/ecdh.c
> +++ b/crypto/ecdh.c
> @@ -30,8 +30,8 @@ static inline struct ecdh_ctx *ecdh_get_ctx(struct crypto_kpp *tfm)
>  static unsigned int ecdh_supported_curve(unsigned int curve_id)
>  {
>         switch (curve_id) {
> -       case ECC_CURVE_NIST_P192: return 3;
> -       case ECC_CURVE_NIST_P256: return 4;
> +       case ECC_CURVE_NIST_P192: return ECC_CURVE_NIST_P192_DIGITS;
> +       case ECC_CURVE_NIST_P256: return ECC_CURVE_NIST_P256_DIGITS;
>         default: return 0;
>         }
>  }
> --
> 2.7.4
>
>
> --
> Kees Cook
> Pixel Security
Herbert Xu April 20, 2018, 4:50 p.m. UTC | #2
On Fri, Mar 30, 2018 at 09:55:44AM -0700, Kees Cook wrote:
> On the quest to remove all VLAs from the kernel[1], this avoids VLAs
> by just using the maximum allocation size (4 bytes) for stack arrays.
> All the VLAs in ecc were either 3 or 4 bytes (or a multiple), so just
> make it 4 bytes all the time. Initialization routines are adjusted to
> check that ndigits does not end up larger than the arrays.
> 
> This includes a removal of the earlier attempt at this fix from
> commit a963834b4742 ("crypto/ecc: Remove stack VLA usage")
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Patch applied.  Thanks.

Patch
diff mbox

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 9c066b5ac12d..815541309a95 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -515,7 +515,7 @@  static void vli_mmod_fast_256(u64 *result, const u64 *product,
 static bool vli_mmod_fast(u64 *result, u64 *product,
 			  const u64 *curve_prime, unsigned int ndigits)
 {
-	u64 tmp[2 * ndigits];
+	u64 tmp[2 * ECC_MAX_DIGITS];
 
 	switch (ndigits) {
 	case 3:
@@ -536,7 +536,7 @@  static bool vli_mmod_fast(u64 *result, u64 *product,
 static void vli_mod_mult_fast(u64 *result, const u64 *left, const u64 *right,
 			      const u64 *curve_prime, unsigned int ndigits)
 {
-	u64 product[2 * ndigits];
+	u64 product[2 * ECC_MAX_DIGITS];
 
 	vli_mult(product, left, right, ndigits);
 	vli_mmod_fast(result, product, curve_prime, ndigits);
@@ -546,7 +546,7 @@  static void vli_mod_mult_fast(u64 *result, const u64 *left, const u64 *right,
 static void vli_mod_square_fast(u64 *result, const u64 *left,
 				const u64 *curve_prime, unsigned int ndigits)
 {
-	u64 product[2 * ndigits];
+	u64 product[2 * ECC_MAX_DIGITS];
 
 	vli_square(product, left, ndigits);
 	vli_mmod_fast(result, product, curve_prime, ndigits);
@@ -560,8 +560,8 @@  static void vli_mod_square_fast(u64 *result, const u64 *left,
 static void vli_mod_inv(u64 *result, const u64 *input, const u64 *mod,
 			unsigned int ndigits)
 {
-	u64 a[ndigits], b[ndigits];
-	u64 u[ndigits], v[ndigits];
+	u64 a[ECC_MAX_DIGITS], b[ECC_MAX_DIGITS];
+	u64 u[ECC_MAX_DIGITS], v[ECC_MAX_DIGITS];
 	u64 carry;
 	int cmp_result;
 
@@ -649,8 +649,8 @@  static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 *z1,
 				      u64 *curve_prime, unsigned int ndigits)
 {
 	/* t1 = x, t2 = y, t3 = z */
-	u64 t4[ndigits];
-	u64 t5[ndigits];
+	u64 t4[ECC_MAX_DIGITS];
+	u64 t5[ECC_MAX_DIGITS];
 
 	if (vli_is_zero(z1, ndigits))
 		return;
@@ -711,7 +711,7 @@  static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 *z1,
 static void apply_z(u64 *x1, u64 *y1, u64 *z, u64 *curve_prime,
 		    unsigned int ndigits)
 {
-	u64 t1[ndigits];
+	u64 t1[ECC_MAX_DIGITS];
 
 	vli_mod_square_fast(t1, z, curve_prime, ndigits);    /* z^2 */
 	vli_mod_mult_fast(x1, x1, t1, curve_prime, ndigits); /* x1 * z^2 */
@@ -724,7 +724,7 @@  static void xycz_initial_double(u64 *x1, u64 *y1, u64 *x2, u64 *y2,
 				u64 *p_initial_z, u64 *curve_prime,
 				unsigned int ndigits)
 {
-	u64 z[ndigits];
+	u64 z[ECC_MAX_DIGITS];
 
 	vli_set(x2, x1, ndigits);
 	vli_set(y2, y1, ndigits);
@@ -750,7 +750,7 @@  static void xycz_add(u64 *x1, u64 *y1, u64 *x2, u64 *y2, u64 *curve_prime,
 		     unsigned int ndigits)
 {
 	/* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
-	u64 t5[ndigits];
+	u64 t5[ECC_MAX_DIGITS];
 
 	/* t5 = x2 - x1 */
 	vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
@@ -791,9 +791,9 @@  static void xycz_add_c(u64 *x1, u64 *y1, u64 *x2, u64 *y2, u64 *curve_prime,
 		       unsigned int ndigits)
 {
 	/* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
-	u64 t5[ndigits];
-	u64 t6[ndigits];
-	u64 t7[ndigits];
+	u64 t5[ECC_MAX_DIGITS];
+	u64 t6[ECC_MAX_DIGITS];
+	u64 t7[ECC_MAX_DIGITS];
 
 	/* t5 = x2 - x1 */
 	vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
@@ -846,9 +846,9 @@  static void ecc_point_mult(struct ecc_point *result,
 			   unsigned int ndigits)
 {
 	/* R0 and R1 */
-	u64 rx[2][ndigits];
-	u64 ry[2][ndigits];
-	u64 z[ndigits];
+	u64 rx[2][ECC_MAX_DIGITS];
+	u64 ry[2][ECC_MAX_DIGITS];
+	u64 z[ECC_MAX_DIGITS];
 	int i, nb;
 	int num_bits = vli_num_bits(scalar, ndigits);
 
@@ -943,13 +943,13 @@  int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
 int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
 {
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
-	u64 priv[ndigits];
+	u64 priv[ECC_MAX_DIGITS];
 	unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
 	unsigned int nbits = vli_num_bits(curve->n, ndigits);
 	int err;
 
 	/* Check that N is included in Table 1 of FIPS 186-4, section 6.1.1 */
-	if (nbits < 160)
+	if (nbits < 160 || ndigits > ARRAY_SIZE(priv))
 		return -EINVAL;
 
 	/*
@@ -988,10 +988,10 @@  int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 {
 	int ret = 0;
 	struct ecc_point *pk;
-	u64 priv[ndigits];
+	u64 priv[ECC_MAX_DIGITS];
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
 
-	if (!private_key || !curve) {
+	if (!private_key || !curve || ndigits > ARRAY_SIZE(priv)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1025,30 +1025,25 @@  int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 {
 	int ret = 0;
 	struct ecc_point *product, *pk;
-	u64 *priv, *rand_z;
+	u64 priv[ECC_MAX_DIGITS];
+	u64 rand_z[ECC_MAX_DIGITS];
+	unsigned int nbytes;
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
 
-	if (!private_key || !public_key || !curve) {
+	if (!private_key || !public_key || !curve ||
+	    ndigits > ARRAY_SIZE(priv) || ndigits > ARRAY_SIZE(rand_z)) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
 
-	rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
-	if (!rand_z) {
-		ret = -ENOMEM;
-		goto kfree_out;
-	}
+	get_random_bytes(rand_z, nbytes);
 
 	pk = ecc_alloc_point(ndigits);
 	if (!pk) {
 		ret = -ENOMEM;
-		goto kfree_out;
+		goto out;
 	}
 
 	product = ecc_alloc_point(ndigits);
@@ -1057,8 +1052,6 @@  int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 		goto err_alloc_product;
 	}
 
-	get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
-
 	ecc_swap_digits(public_key, pk->x, ndigits);
 	ecc_swap_digits(&public_key[ndigits], pk->y, ndigits);
 	ecc_swap_digits(private_key, priv, ndigits);
@@ -1073,9 +1066,6 @@  int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 	ecc_free_point(product);
 err_alloc_product:
 	ecc_free_point(pk);
-kfree_out:
-	kzfree(priv);
-	kzfree(rand_z);
 out:
 	return ret;
 }
diff --git a/crypto/ecc.h b/crypto/ecc.h
index e4fd4492c765..f75a86baa3bd 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -26,7 +26,9 @@ 
 #ifndef _CRYPTO_ECC_H
 #define _CRYPTO_ECC_H
 
-#define ECC_MAX_DIGITS	4 /* 256 */
+#define ECC_CURVE_NIST_P192_DIGITS  3
+#define ECC_CURVE_NIST_P256_DIGITS  4
+#define ECC_MAX_DIGITS              ECC_CURVE_NIST_P256_DIGITS
 
 #define ECC_DIGITS_TO_BYTES_SHIFT 3
 
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index 3aca0933ec44..3f91ef13c8c6 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -30,8 +30,8 @@  static inline struct ecdh_ctx *ecdh_get_ctx(struct crypto_kpp *tfm)
 static unsigned int ecdh_supported_curve(unsigned int curve_id)
 {
 	switch (curve_id) {
-	case ECC_CURVE_NIST_P192: return 3;
-	case ECC_CURVE_NIST_P256: return 4;
+	case ECC_CURVE_NIST_P192: return ECC_CURVE_NIST_P192_DIGITS;
+	case ECC_CURVE_NIST_P256: return ECC_CURVE_NIST_P256_DIGITS;
 	default: return 0;
 	}
 }