Re: [PATCH v2] crypto: ecc - Prevent ecc_digits_from_bytes from reading too many bytes

[Jarkko Sakkinen]

On Mon Apr 29, 2024 at 7:13 PM EEST, Stefan Berger wrote:
> Prevent ecc_digits_from_bytes from reading too many bytes from the input
> byte array in case an insufficient number of bytes is provided to fill the
> output digit array of ndigits. Therefore, initialize the most significant
> digits with 0 to avoid trying to read too many bytes later on. Convert the
> function into a regular function since it is getting too big for an inline
> function.
>
> If too many bytes are provided on the input byte array the extra bytes
> are ignored since the input variable 'ndigits' limits the number of digits
> that will be filled.
>
> Fixes: d67c96fb97b5 ("crypto: ecdsa - Convert byte arrays with key coordinates to digits")
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>
> ---
>
> v2:
>  - un-inline function
>  - use memset
> ---
>  crypto/ecc.c                  | 22 ++++++++++++++++++++++
>  include/crypto/internal/ecc.h | 15 ++-------------
>  2 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index c1d2e884be1e..fe761256e335 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -68,6 +68,28 @@ const struct ecc_curve *ecc_get_curve(unsigned int curve_id)
>  }
>  EXPORT_SYMBOL(ecc_get_curve);
>  

Just a minor nit:

For exported symbol you need to document the function,including
the parameters [1].

> +void ecc_digits_from_bytes(const u8 *in, unsigned int nbytes,
> +			   u64 *out, unsigned int ndigits)
> +{
> +	int diff = ndigits - DIV_ROUND_UP(nbytes, sizeof(u64));
> +	unsigned int o = nbytes & 7;
> +	__be64 msd = 0;
> +
> +	/* diff > 0: not enough input bytes: set most significant digits to 0 */
> +	if (diff > 0) {
> +		ndigits -= diff;
> +		memset(&out[ndigits - 1], 0, diff * sizeof(u64));
> +	}
> +
> +	if (o) {
> +		memcpy((u8 *)&msd + sizeof(msd) - o, in, o);
> +		out[--ndigits] = be64_to_cpu(msd);
> +		in += o;
> +	}
> +	ecc_swap_digits(in, out, ndigits);
> +}
> +EXPORT_SYMBOL(ecc_digits_from_bytes);
> +
>  static u64 *ecc_alloc_digits_space(unsigned int ndigits)
>  {
>  	size_t len = ndigits * sizeof(u64);
> diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
> index 7ca1f463d1ec..f7e75e1e71f3 100644
> --- a/include/crypto/internal/ecc.h
> +++ b/include/crypto/internal/ecc.h
> @@ -64,19 +64,8 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit
>   * @out       Output digits array
>   * @ndigits:  Number of digits to create from byte array
>   */
> -static inline void ecc_digits_from_bytes(const u8 *in, unsigned int nbytes,
> -					 u64 *out, unsigned int ndigits)
> -{
> -	unsigned int o = nbytes & 7;
> -	__be64 msd = 0;
> -
> -	if (o) {
> -		memcpy((u8 *)&msd + sizeof(msd) - o, in, o);
> -		out[--ndigits] = be64_to_cpu(msd);
> -		in += o;
> -	}
> -	ecc_swap_digits(in, out, ndigits);
> -}
> +void ecc_digits_from_bytes(const u8 *in, unsigned int nbytes,
> +			   u64 *out, unsigned int ndigits);
>  
>  /**
>   * ecc_is_key_valid() - Validate a given ECDH private key

[1] https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

BR, Jarkko