| Message ID | 20240429161316.3146626-1-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] crypto: ecc - Prevent ecc_digits_from_bytes from reading too many bytes | expand |
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@linux.ibm.com> > > --- > > 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
On 4/29/24 12:47, Jarkko Sakkinen wrote: > 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@linux.ibm.com> >> >> --- >> >> 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]. Like other functions, the ecc_digits_from_bytes also still/already has the documentation in the header file: /** * ecc_digits_from_bytes() - Create ndigits-sized digits array from byte array * @in: Input byte array * @nbytes Size of input byte array * @out Output digits array * @ndigits: Number of digits to create from byte array */ void ecc_digits_from_bytes(const u8 *in, unsigned int nbytes, u64 *out, unsigned int ndigits); Should be ok?
On Mon Apr 29, 2024 at 7:57 PM EEST, Stefan Berger wrote: > > > On 4/29/24 12:47, Jarkko Sakkinen wrote: > > 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@linux.ibm.com> > >> > >> --- > >> > >> 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]. > > Like other functions, the ecc_digits_from_bytes also still/already has > the documentation in the header file: > > /** > * ecc_digits_from_bytes() - Create ndigits-sized digits array from > byte array > * @in: Input byte array > * @nbytes Size of input byte array > * @out Output digits array > * @ndigits: Number of digits to create from byte array > */ > void ecc_digits_from_bytes(const u8 *in, unsigned int nbytes, > u64 *out, unsigned int ndigits); > > Should be ok? I think it should be OK, or at least documentation has not denied doing that and gives example how to import from header files: https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html Just had not encountered that before so that said Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
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); +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
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@linux.ibm.com> --- v2: - un-inline function - use memset --- crypto/ecc.c | 22 ++++++++++++++++++++++ include/crypto/internal/ecc.h | 15 ++------------- 2 files changed, 24 insertions(+), 13 deletions(-)