[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-tegra
Subject: RE: [cbootimage PATCH v1 5/8] Fix some issues found in libmcrypto
From: Jimmy Zhang <jimmzhang () nvidia ! com>
Date: 2015-09-22 1:11:44
Message-ID: 5ab64e0d63f34f8abdc159703ea79fd5 () HQMAIL103 ! nvidia ! com
[Download RAW message or body]
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Monday, September 21, 2015 3:09 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra@vger.kernel.org
> Subject: Re: [cbootimage PATCH v1 5/8] Fix some issues found in libmcrypto
>
> On 09/02/2015 03:19 PM, Jimmy Zhang wrote:
> > Libmcrypto can't be used without these fixes.
>
> This patch should appear immediately after the patch which adds libmcrypto
> into cbootimage then, before any patch that starts using it.
>
OK.
> Have you sent these patches upstream for inclusion in libmcrypto?
No. I am not whether these changes are generic or not.
>
> Some description of the fixes and justification for them should be included in
> the commit description.
>
OK.
> > diff --git a/src/libm/bigdigits.h b/src/libm/bigdigits.h
>
> > /* Define type of DIGIT here */
> > -typedef unsigned long DIGIT_T;
> > +typedef unsigned int DIGIT_T;
>
> I wonder if this is running afoul of 32-bit-vs-64-bit system assumptions? Does
> this e.g. fix the build on 64-bit but break it on 32-bit? What symptom does
> this solve? Would it make sense to typedef to e.g. uint32_t if a specific size is
> required?
>
> > diff --git a/src/libm/common.c b/src/libm/common.c
>
> > @@ -46,11 +46,11 @@ void mcrypto_dump(char *desc, BYTE *p, UINT len)
> > #ifdef MCRYPTO_DEBUG
> > UINT i = 0;
> >
> > - printf("[%s]\n", desc);
> > + printf("[%s(%d)]\n", desc, len);
> > while (len--) {
> > if ((i % 20) == 0 && i)
> > printf("\n");
> > - fprintf(stderr, "%02x ", p[len]);
> > + fprintf(stderr, "%02x ", p[i]);
>
> While perhaps a valid fix, that looks like debug spew cleanup, so not strictly
> "required".
>
> > diff --git a/src/libm/mpModulo.c b/src/libm/mpModulo.c index
> > c929dd5a2c02..cff60d173e8b 100644
>
> > +/* TODO: add support for MCRYPTO_BARRET */ #define
> > +MCRYPTO_TRIVIAL_DIVISION
>
> The libmcrypto Makefile implies this should be solved by adding -
> DMCRYPTO_TRIVIAL_DIVISION to the build command-line instead.
>
> > diff --git a/src/libm/mpMultiply.c b/src/libm/mpMultiply.c
>
> > +/* TODO: add support for MCRYPTO_FFT_MUL */ #define
> > +MCRYPTO_SCHOOL_BOOK
>
> Same here.
>
> > int mpMultiply(DIGIT_T w[], const DIGIT_T u[], const DIGIT_T v[], UINT
> ndigits)
> > {
> > -#ifdef MCRYPTO_SCHOOL_BOOK
> > +#ifdef MCRYPTO_SCHOOL_BOOK
>
> Unnecessary whitespace change.
>
> > diff --git a/src/libm/pkcs1-rsa.c b/src/libm/pkcs1-rsa.c
>
> > +/* cbootimage header */
> > +#include "crypto.h"
> > +
> > /* Internal Functions - Forward Declaration */
> > static void memxor(BYTE *c, BYTE *a, BYTE *b, UINT len);
> > /* Perform c = a XOR b */
> > @@ -59,6 +62,15 @@ static int GenRsaPrime(DIGIT_T p[], UINT ndigits)
> > return 0;
> > }
> >
> > +static
> > +UINT SwapBytesInNvU32(const UINT Value) {
> > + UINT Tmp = (Value << 16) | (Value >> 16); /* Swap halves */
> > + /* Swap bytes pairwise */
> > + Tmp = ((Tmp >> 8) & 0x00ff00ff) | ((Tmp & 0x00ff00ff) << 8);
> > + return (Tmp);
>
> No need for ().
>
> Does "NvU32" refer to the NvU32 type? That's not something that should
> exist within libmcrypto since it's not NV-specific code.
>
> > @@ -91,8 +103,8 @@ static int MGF1(UINT hid, BYTE *seed, UINT seedlen,
> > BYTE *mask, UINT masklen)
> >
> > for(i=0;i<n;i++) {
> > /* Constructing Hash Input */
> > - memcpy(data+seedlen, &i, 4);
> > -
> > + *(UINT *)(data+seedlen) = SwapBytesInNvU32(i);
> > +
>
> Why?
I don't fully understand the code. The change is based on Tegrasign.
>
> > @@ -113,7 +125,6 @@ static int MGF1(UINT hid, BYTE *seed, UINT
> seedlen, BYTE *mask, UINT masklen)
> > }
> >
> > /* Main Functions */
> > -
> > int PKCS1_RSA_GenKey(PKCS1_RSA_PUBLIC_KEY *spk,
> > PKCS1_RSA_PRIVATE_KEY *ssk, UINT mod_len)
>
> Unrelated whitespace change.
>
> > @@ -511,14 +522,19 @@ int
> PKCS1_RSASSA_PSS_SIGN(PKCS1_RSA_PRIVATE_KEY *ssk, UINT hid, BYTE
> *m, UINT ml
> > em = (BYTE *)malloc(NBYTE(ssk->len));
> >
> > /* PSS Encoding */
> > - if((ret=PKCS1_EMSA_PSS_ENCODE(hid, m, mlen, slen, em,
> NBYTE(ssk->len)))!=ERR_OK) {
> > + if((ret = PKCS1_EMSA_PSS_ENCODE(hid, m, mlen, slen, em,
> NBYTE(ssk->len)))
> > + != ERR_OK) {
>
> Unrelated whitespace change.
>
> > free(em);
> > + printf("Error: encoding failed\n");
> > return ret;
>
OK.
> Why? The code already returns an error, and a library really shouldn't be
> spewing error messages over stdout unless asked.
>
> > + SwapEndianness(em, NBYTE(ssk->len), em);
> > + mcrypto_dump("PSS_SIGN: Encoded Message", em, NBYTE(ssk-
> > len));
> > +
> > /* Signing */
> > ret = PKCS1_RSASP1(ssk, (DIGIT_T*)em, (DIGIT_T*)s);
> > - mcrypto_dump("Signature",(BYTE *)s, NBYTE(ssk->len));
> > + mcrypto_dump("PSS_SIGN: Signature",(BYTE *)s, NBYTE(ssk->len));
>
> Why?
Again, it is based on Tegrasign. Otherwise, the signature value generated mismatches \
what chip expects.
>
> > +/*
> > + * hid: hash id
> > + * m: message buffer
> > + * mlen: message length
> > + * slen: signature length
> > + * em: encoded message (from hash)
> > + * emlen: encoded message length -> 256 */
> > int PKCS1_EMSA_PSS_ENCODE(UINT hid, BYTE *m, UINT mlen, UINT slen,
> BYTE *em, UINT emlen)
> > {
> > /* PSS Encoding */
> > @@ -568,31 +592,34 @@ int PKCS1_EMSA_PSS_ENCODE(UINT hid, BYTE
> *m, UINT mlen, UINT slen, BYTE *em, UIN
> > return ERR_UNKNOWN_HASH;
> >
> > /* Computing Hash of m */
> > - mcrypto_dump("PSS Encoding: Message", m, mlen);
>
> Why?
>
> > H = (BYTE *)malloc(hlen);
> > if((ret = Hash(hid, m, mlen, H))!=0) {
> > free(H);
> > -
>
> Unrelated whitespace/formatting change. There are others too, but I won't
> call out each individually.
>
> > return ret;
> > }
> >
> > mcrypto_dump("PSS Encoding: Hashed Message", H, hlen);
> >
> > + /* BUG FIX */
> > + /* slen is 256 that causes the condition below failed */
> > + /* FIX: set slen to hash length */
> > + slen = hlen;
> > +
>
> slen is a parameter to this function. Why not just pass the correct value to
> the function?
>
This is a bug. slen is signature length. The value passed in is 256 and is correct. \
What I made here is a HACK.
Maybe the statement below should be corrected.
> > /* Length checking */
> > - if(emlen<(hlen+slen+2)) {
> > + if(emlen<(hlen+slen+2)) { /* emlen: 256, hlen: 32, slen: 32 */
>
> Are those values always true for /any/ use of this function?
>
> > /* Generating salt and constructing M */
> > salt = (BYTE *)malloc(slen);
> > - GenSeed(salt, slen);
> > - mcrypto_dump("PSS Encoding: Salt", salt, slen);
> > + /* GenSeed(salt, slen); */
> > + memset(salt, 0xFF, slen);
>
> This looks like a hack for some specific use-case. We shouldn't change the
> semantics of libmcrypto in this way.
>
Again, I do not understand neither. But, it is how Tegrasign does.
> > @@ -629,11 +656,18 @@ int PKCS1_EMSA_PSS_ENCODE(UINT hid, BYTE
> *m, UINT mlen, UINT slen, BYTE *em, UIN
> > mcrypto_dump("PSS Encoding: maskedDB", maskedDB, emlen-hlen-
> 1);
> >
> > /* Constructing encoded message, em */
> > + maskedDB[0] &= ~(0xFF << (8 - 1));
>
> Why?
>
Same as above.
> > memcpy(em, maskedDB, emlen-hlen-1);
> > memcpy(em+emlen-hlen-1, H, hlen);
> > em[emlen-1] = 0xbc;
> > - mcrypto_dump("PSS Encoding: Encoded Message", em, emlen);
>
> Why?
>
> > + /* added: free memory H, M, DB, ... */
> > + free(H);
> > + free(M);
> > + free(salt);
> > + free(maskedDB);
> > + free(DB);
>
> That comment doesn't add any useful information.
>
Forgot to remove comments.
> > -int LoadPublicKey(char *fname, PKCS1_RSA_PUBLIC_KEY *spk) -{
> ... (entire function body removed)
> > -}
> > -
> > -int LoadPrivateKey(char *fname, PKCS1_RSA_PRIVATE_KEY *ssk) -{
> ... (entire function body removed)
> > -}
>
> Why?
>
> > diff --git a/src/libm/pkcs1-rsa.h b/src/libm/pkcs1-rsa.h
>
> > -#define PKCS1_MAX_LINE_LEN 346 /* for reading parameter file
> */
> > +#define PKCS1_MAX_NUM_KEYS 8 /* number of key
> components */
> > +#define PKCS1_MAX_LINE_LEN 512 /* for reading parameter file
> */
>
> PKCS1_MAX_NUM_KEYS doesn't seem to be used anywhere.
OK. Will do more clean up.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic