linux-kernel - Re: [PATCH] crypto: ecc - Protect ecc_digits_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 29 Apr 2024 16:12:54 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Stefan Berger" <stefanb@...ux.ibm.com>, "Lukas Wunner"
 <lukas@...ner.de>
Cc: <keyrings@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
 <herbert@...dor.apana.org.au>, <davem@...emloft.net>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] crypto: ecc - Protect ecc_digits_from_bytes from
 reading too many bytes

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@...ux.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