| Message ID | 20180307215615.GA18928@beast (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi, Kees, On 03/07/2018 11:56 PM, Kees Cook wrote: > On the quest to remove all VLAs from the kernel[1], this switches to > a pair of kmalloc regions instead of using the stack. This also moves > the get_random_bytes() after all allocations (and drops the needless > "nbytes" variable). > > [1] https://lkml.org/lkml/2018/3/7/621 > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > crypto/ecc.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/crypto/ecc.c b/crypto/ecc.c > index 18f32f2a5e1c..5bfa63603da0 100644 > --- a/crypto/ecc.c > +++ b/crypto/ecc.c > @@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits, > { > int ret = 0; > struct ecc_point *product, *pk; > - u64 priv[ndigits]; > - u64 rand_z[ndigits]; > - unsigned int nbytes; > + u64 *priv, *rand_z; > const struct ecc_curve *curve = ecc_get_curve(curve_id); > > if (!private_key || !public_key || !curve) { > @@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits, > goto out; > } > > - nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; > + priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; > + goto out; > + } > > - get_random_bytes(rand_z, nbytes); > + rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL); > + if (!rand_z) { > + ret = -ENOMEM; > + goto kfree_out; > + } > > pk = ecc_alloc_point(ndigits); > if (!pk) { > ret = -ENOMEM; > - goto out; > + goto kfree_out; > } > > product = ecc_alloc_point(ndigits); > @@ -1051,6 +1057,8 @@ 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); > @@ -1065,6 +1073,9 @@ 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: > + kfree(priv); I think we should use kzfree here. > + kfree(rand_z); Probably here too. Looks like there are few intermediate buffers in ecc that should be zeroized as well. Best, ta > out: > return ret; > } >
On Thu, Mar 8, 2018 at 1:43 AM, Tudor Ambarus <tudor.ambarus@microchip.com> wrote: > Hi, Kees, > > > On 03/07/2018 11:56 PM, Kees Cook wrote: >> >> On the quest to remove all VLAs from the kernel[1], this switches to >> a pair of kmalloc regions instead of using the stack. This also moves >> the get_random_bytes() after all allocations (and drops the needless >> "nbytes" variable). >> >> [1] https://lkml.org/lkml/2018/3/7/621 >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> crypto/ecc.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/crypto/ecc.c b/crypto/ecc.c >> index 18f32f2a5e1c..5bfa63603da0 100644 >> --- a/crypto/ecc.c >> +++ b/crypto/ecc.c >> @@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, >> unsigned int ndigits, >> { >> int ret = 0; >> struct ecc_point *product, *pk; >> - u64 priv[ndigits]; >> - u64 rand_z[ndigits]; >> - unsigned int nbytes; >> + u64 *priv, *rand_z; >> const struct ecc_curve *curve = ecc_get_curve(curve_id); >> if (!private_key || !public_key || !curve) { >> @@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int >> curve_id, unsigned int ndigits, >> goto out; >> } >> - nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; >> + priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL); >> + if (!priv) { >> + ret = -ENOMEM; >> + goto out; >> + } >> - get_random_bytes(rand_z, nbytes); >> + rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL); >> + if (!rand_z) { >> + ret = -ENOMEM; >> + goto kfree_out; >> + } >> pk = ecc_alloc_point(ndigits); >> if (!pk) { >> ret = -ENOMEM; >> - goto out; >> + goto kfree_out; >> } >> product = ecc_alloc_point(ndigits); >> @@ -1051,6 +1057,8 @@ 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); >> @@ -1065,6 +1073,9 @@ 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: >> + kfree(priv); > > > I think we should use kzfree here. > >> + kfree(rand_z); > > > Probably here too. Ah yeah, good idea. I'll send a v2. > Looks like there are few intermediate buffers in ecc > that should be zeroized as well. Can you send a patch for those? Thanks! -Kees > > Best, > ta >> >> out: >> return ret; >> } >> >
On 03/08/2018 11:55 PM, Kees Cook wrote: >> Looks like there are few intermediate buffers in ecc >> that should be zeroized as well. > Can you send a patch for those? Yeah, I'll take a look. Best, ta
diff --git a/crypto/ecc.c b/crypto/ecc.c index 18f32f2a5e1c..5bfa63603da0 100644 --- a/crypto/ecc.c +++ b/crypto/ecc.c @@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits, { int ret = 0; struct ecc_point *product, *pk; - u64 priv[ndigits]; - u64 rand_z[ndigits]; - unsigned int nbytes; + u64 *priv, *rand_z; const struct ecc_curve *curve = ecc_get_curve(curve_id); if (!private_key || !public_key || !curve) { @@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits, goto out; } - nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; + priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL); + if (!priv) { + ret = -ENOMEM; + goto out; + } - get_random_bytes(rand_z, nbytes); + rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL); + if (!rand_z) { + ret = -ENOMEM; + goto kfree_out; + } pk = ecc_alloc_point(ndigits); if (!pk) { ret = -ENOMEM; - goto out; + goto kfree_out; } product = ecc_alloc_point(ndigits); @@ -1051,6 +1057,8 @@ 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); @@ -1065,6 +1073,9 @@ 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: + kfree(priv); + kfree(rand_z); out: return ret; }
On the quest to remove all VLAs from the kernel[1], this switches to a pair of kmalloc regions instead of using the stack. This also moves the get_random_bytes() after all allocations (and drops the needless "nbytes" variable). [1] https://lkml.org/lkml/2018/3/7/621 Signed-off-by: Kees Cook <keescook@chromium.org> --- crypto/ecc.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)