* [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
@ 2024-04-26 22:55 Stefan Berger
2024-04-28 22:12 ` Jarkko Sakkinen
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Berger @ 2024-04-26 22:55 UTC (permalink / raw)
To: keyrings, linux-crypto, herbert, davem
Cc: linux-kernel, lukas, jarkko, Stefan Berger
Protect 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.
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>
---
include/crypto/internal/ecc.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
index 7ca1f463d1ec..56215f14ff96 100644
--- a/include/crypto/internal/ecc.h
+++ b/include/crypto/internal/ecc.h
@@ -67,9 +67,16 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit
static inline 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 */
+ while (diff > 0) {
+ out[--ndigits] = 0;
+ diff--;
+ }
+
if (o) {
memcpy((u8 *)&msd + sizeof(msd) - o, in, o);
out[--ndigits] = be64_to_cpu(msd);
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-04-26 22:55 [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes Stefan Berger
@ 2024-04-28 22:12 ` Jarkko Sakkinen
2024-04-29 3:30 ` Lukas Wunner
0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-04-28 22:12 UTC (permalink / raw)
To: Stefan Berger, keyrings, linux-crypto, herbert, davem; +Cc: linux-kernel, lukas
On Sat Apr 27, 2024 at 1:55 AM EEST, Stefan Berger wrote:
> Protect 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.
>
> 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>
> ---
> include/crypto/internal/ecc.h | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
> index 7ca1f463d1ec..56215f14ff96 100644
> --- a/include/crypto/internal/ecc.h
> +++ b/include/crypto/internal/ecc.h
> @@ -67,9 +67,16 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit
> static inline 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 */
> + while (diff > 0) {
> + out[--ndigits] = 0;
> + diff--;
> + }
Could be just trivial for-loop:
for (i = 0; i < diff; i++)
out[--ndigits] = 0;
Or also simpler while-loop could work:
while (diff-- > 0)
out[--ndigits] = 0;
BR, Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-04-28 22:12 ` Jarkko Sakkinen
@ 2024-04-29 3:30 ` Lukas Wunner
2024-04-29 10:14 ` Jarkko Sakkinen
0 siblings, 1 reply; 9+ messages in thread
From: Lukas Wunner @ 2024-04-29 3:30 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Stefan Berger, keyrings, linux-crypto, herbert, davem,
linux-kernel
On Mon, Apr 29, 2024 at 01:12:00AM +0300, Jarkko Sakkinen wrote:
> On Sat Apr 27, 2024 at 1:55 AM EEST, Stefan Berger wrote:
> > Protect 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.
> >
> > 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>
> > ---
> > include/crypto/internal/ecc.h | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
> > index 7ca1f463d1ec..56215f14ff96 100644
> > --- a/include/crypto/internal/ecc.h
> > +++ b/include/crypto/internal/ecc.h
> > @@ -67,9 +67,16 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit
> > static inline 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 */
> > + while (diff > 0) {
> > + out[--ndigits] = 0;
> > + diff--;
> > + }
>
> Could be just trivial for-loop:
>
> for (i = 0; i < diff; i++)
> out[--ndigits] = 0;
>
> Or also simpler while-loop could work:
>
> while (diff-- > 0)
> out[--ndigits] = 0;
Or just use memset(), which uses optimized instructions on many arches.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-04-29 3:30 ` Lukas Wunner
@ 2024-04-29 10:14 ` Jarkko Sakkinen
2024-04-29 11:11 ` Stefan Berger
2024-05-03 10:30 ` Herbert Xu
0 siblings, 2 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-04-29 10:14 UTC (permalink / raw)
To: Lukas Wunner
Cc: Stefan Berger, keyrings, linux-crypto, herbert, davem,
linux-kernel
On Mon Apr 29, 2024 at 6:30 AM EEST, Lukas Wunner wrote:
> On Mon, Apr 29, 2024 at 01:12:00AM +0300, Jarkko Sakkinen wrote:
> > On Sat Apr 27, 2024 at 1:55 AM EEST, Stefan Berger wrote:
> > > Protect 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.
> > >
> > > 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>
> > > ---
> > > include/crypto/internal/ecc.h | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
> > > index 7ca1f463d1ec..56215f14ff96 100644
> > > --- a/include/crypto/internal/ecc.h
> > > +++ b/include/crypto/internal/ecc.h
> > > @@ -67,9 +67,16 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit
> > > static inline 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 */
> > > + while (diff > 0) {
> > > + out[--ndigits] = 0;
> > > + diff--;
> > > + }
> >
> > Could be just trivial for-loop:
> >
> > for (i = 0; i < diff; i++)
> > out[--ndigits] = 0;
> >
> > Or also simpler while-loop could work:
> >
> > while (diff-- > 0)
> > out[--ndigits] = 0;
>
> Or just use memset(), which uses optimized instructions on many arches.
Yeah, sure, that would be even better, or even memzero_explicit()?
BR, Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-04-29 10:14 ` Jarkko Sakkinen
@ 2024-04-29 11:11 ` Stefan Berger
2024-04-29 13:12 ` Jarkko Sakkinen
2024-05-03 10:30 ` Herbert Xu
1 sibling, 1 reply; 9+ messages in thread
From: Stefan Berger @ 2024-04-29 11:11 UTC (permalink / raw)
To: Jarkko Sakkinen, Lukas Wunner
Cc: keyrings, linux-crypto, herbert, davem, linux-kernel
On 4/29/24 06:14, Jarkko Sakkinen wrote:
> On Mon Apr 29, 2024 at 6:30 AM EEST, Lukas Wunner wrote:
>> On Mon, Apr 29, 2024 at 01:12:00AM +0300, Jarkko Sakkinen wrote:
>>> On Sat Apr 27, 2024 at 1:55 AM EEST, Stefan Berger wrote:
>>>> Protect 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.
>>>>
>>>> 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>
>>>> ---
>>>> include/crypto/internal/ecc.h | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
>>>> index 7ca1f463d1ec..56215f14ff96 100644
>>>> --- a/include/crypto/internal/ecc.h
>>>> +++ b/include/crypto/internal/ecc.h
>>>> @@ -67,9 +67,16 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit
>>>> static inline 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 */
>>>> + while (diff > 0) {
>>>> + out[--ndigits] = 0;
>>>> + diff--;
>>>> + }
>>>
>>> Could be just trivial for-loop:
>>>
>>> for (i = 0; i < diff; i++)
>>> out[--ndigits] = 0;
>>>
>>> Or also simpler while-loop could work:
>>>
>>> while (diff-- > 0)
>>> out[--ndigits] = 0;
>>
>> Or just use memset(), which uses optimized instructions on many arches.
>
> Yeah, sure, that would be even better, or even memzero_explicit()?
Thanks. The function isn't getting too big for an inline?
>
> BR, Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-04-29 11:11 ` Stefan Berger
@ 2024-04-29 13:12 ` Jarkko Sakkinen
0 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-04-29 13:12 UTC (permalink / raw)
To: Stefan Berger, Lukas Wunner
Cc: keyrings, linux-crypto, herbert, davem, linux-kernel
On Mon Apr 29, 2024 at 2:11 PM EEST, Stefan Berger wrote:
>
>
> On 4/29/24 06:14, Jarkko Sakkinen wrote:
> > On Mon Apr 29, 2024 at 6:30 AM EEST, Lukas Wunner wrote:
> >> On Mon, Apr 29, 2024 at 01:12:00AM +0300, Jarkko Sakkinen wrote:
> >>> On Sat Apr 27, 2024 at 1:55 AM EEST, Stefan Berger wrote:
> >>>> Protect 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.
> >>>>
> >>>> 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>
> >>>> ---
> >>>> include/crypto/internal/ecc.h | 7 +++++++
> >>>> 1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
> >>>> index 7ca1f463d1ec..56215f14ff96 100644
> >>>> --- a/include/crypto/internal/ecc.h
> >>>> +++ b/include/crypto/internal/ecc.h
> >>>> @@ -67,9 +67,16 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit
> >>>> static inline 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 */
> >>>> + while (diff > 0) {
> >>>> + out[--ndigits] = 0;
> >>>> + diff--;
> >>>> + }
> >>>
> >>> Could be just trivial for-loop:
> >>>
> >>> for (i = 0; i < diff; i++)
> >>> out[--ndigits] = 0;
> >>>
> >>> Or also simpler while-loop could work:
> >>>
> >>> while (diff-- > 0)
> >>> out[--ndigits] = 0;
> >>
> >> Or just use memset(), which uses optimized instructions on many arches.
> >
> > Yeah, sure, that would be even better, or even memzero_explicit()?
>
> Thanks. The function isn't getting too big for an inline?
Hmm... so as far as I'm concerned you pick what works for you. Just
was pointing out at it would make to simplify the original a bit :-)
BR, Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-04-29 10:14 ` Jarkko Sakkinen
2024-04-29 11:11 ` Stefan Berger
@ 2024-05-03 10:30 ` Herbert Xu
2024-05-03 23:49 ` Jarkko Sakkinen
1 sibling, 1 reply; 9+ messages in thread
From: Herbert Xu @ 2024-05-03 10:30 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Lukas Wunner, Stefan Berger, keyrings, linux-crypto, davem,
linux-kernel
On Mon, Apr 29, 2024 at 01:14:15PM +0300, Jarkko Sakkinen wrote:
>
> Yeah, sure, that would be even better, or even memzero_explicit()?
memzero_explicit should only be used for stack memory.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-05-03 10:30 ` Herbert Xu
@ 2024-05-03 23:49 ` Jarkko Sakkinen
2024-05-03 23:51 ` Jarkko Sakkinen
0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-05-03 23:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Lukas Wunner, Stefan Berger, keyrings, linux-crypto, davem,
linux-kernel
On Fri May 3, 2024 at 1:30 PM EEST, Herbert Xu wrote:
> On Mon, Apr 29, 2024 at 01:14:15PM +0300, Jarkko Sakkinen wrote:
> >
> > Yeah, sure, that would be even better, or even memzero_explicit()?
>
> memzero_explicit should only be used for stack memory.
BTW, is this in kernel documentation? It's a guideline really
and would be nice to have reminder, that's all.
BR, Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes
2024-05-03 23:49 ` Jarkko Sakkinen
@ 2024-05-03 23:51 ` Jarkko Sakkinen
0 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-05-03 23:51 UTC (permalink / raw)
To: Jarkko Sakkinen, Herbert Xu
Cc: Lukas Wunner, Stefan Berger, keyrings, linux-crypto, davem,
linux-kernel
On Sat May 4, 2024 at 2:49 AM EEST, Jarkko Sakkinen wrote:
> On Fri May 3, 2024 at 1:30 PM EEST, Herbert Xu wrote:
> > On Mon, Apr 29, 2024 at 01:14:15PM +0300, Jarkko Sakkinen wrote:
> > >
> > > Yeah, sure, that would be even better, or even memzero_explicit()?
> >
> > memzero_explicit should only be used for stack memory.
>
> BTW, is this in kernel documentation? It's a guideline really
> and would be nice to have reminder, that's all.
Any case makes sense and is pretty obvious now that you said it.
BR, Jarkko
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-05-03 23:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-26 22:55 [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from reading too many bytes Stefan Berger
2024-04-28 22:12 ` Jarkko Sakkinen
2024-04-29 3:30 ` Lukas Wunner
2024-04-29 10:14 ` Jarkko Sakkinen
2024-04-29 11:11 ` Stefan Berger
2024-04-29 13:12 ` Jarkko Sakkinen
2024-05-03 10:30 ` Herbert Xu
2024-05-03 23:49 ` Jarkko Sakkinen
2024-05-03 23:51 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).