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.
Have you sent these patches upstream for inclusion in libmcrypto?
Some description of the fixes and justification for them should be
included in the commit description.
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?
@@ -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;
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?
+/*
+ * 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?
/* 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.
@@ -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?
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.
-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.
--
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
