RE: [cbootimage PATCH v1 5/8] Fix some issues found in libmcrypto

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Stephen Warren [mailto:swarren@xxxxxxxxxxxxx]
> Sent: Monday, September 21, 2015 3:09 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux