> -----Original Message----- > From: Stephen Warren [mailto:swarren@xxxxxxxxxxxxx] > Sent: Monday, September 21, 2015 2:41 PM > To: Jimmy Zhang > Cc: Allen Martin; Stephen Warren; linux-tegra@xxxxxxxxxxxxxxx > Subject: Re: [cbootimage PATCH v1 4/8] Add new configuration keyword > "PkcKey" > > On 09/02/2015 03:19 PM, Jimmy Zhang wrote: > > Use "PkcKey" to specify rsa key filename and load in rsa private keys > > from file. > > > > When keyword "PkcKey" is present, rsa pss signatures are generated for > > both bootloader and bct. rsa pubkey will also be filled into bct. > > Oh, so the config file can either specify a literal value for all the hashes, or > specify a key and have cbootimage generate them? It would be good to add > some form of high-level documentation of the use-cases into README.txt > OK. > > PkcKey syntax: > > > > PkcKey = <rsa_key_filename> [, --save]; > > > > Examples: > > > > PkcKey = rsa_priv.txt; > > Load in keys from file rsa_priv.txt > > > > PkcKey = rsa_priv.pem, --save; > > Load in keys from file rsa_priv.pem and save pubkey and pubkey hash > > value to file pubkey.mod and pubkey.sha respectively. > > Can't the output filenames be either specified in the config file, or derived > from the filename passed to PkcKey (so rsa_priv.pem -> rsa_priv.mod, > rsa_priv.sha, rather than presumably hard-coding "pubkey.mod", > "pubkey.sha")? > OK. > > Two key formats are supported. > > 1. Polar SSL format > > P = ... > > Q = ... > > > > 2. Open SSL format > > -----BEGIN RSA PRIVATE KEY----- > > ... > > -----END RSA PRIVATE KEY----- > > Do we need to support two formats; can't the openssl (or other) cmdline > application convert the data file between the formats to reduce the > complexity in cbootimage? > OK. Then it is Open SSL format. > > diff --git a/src/cbootimage.h b/src/cbootimage.h > > > +typedef enum > > +{ > > + false = 0, > > + true = 1, > > +} bool; > I can try again. Compiler complains for missing type bool. > This is a standard type; the definition should come from a standard system > header file. > > > > @@ -110,6 +117,7 @@ typedef struct build_image_context_rec > > > > char *bct_filename; > > char *rsa_filename; > > + void *pkckey; > > Please expand on what this variable means (comment or better field name). Will remove "rsa_filename". > > > diff --git a/src/crypto.c b/src/crypto.c > > > - * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved. > > + * Copyright (c) 2015, NVIDIA CORPORATION. All rights reserved. > > Please don't delete old copyright dates, but just add to them. i.e. > "2012, 2015". > > > +#include "libm/pkcs1-rsa.h" > > +#include "libm/hash.h" > > I would expect libm/include/ to be in the #include path via a -I > directive, so those would be: > > #include <mcrypto/pkcs1-rsa.h> > #include <mcrypto/hash.h> > > (At least the examples that ship with libmcrypto appear to expect the > library gets installed with the <libmcrypto/> path available, so we > should match that in any code using libmcrypto) > > > +static u_int8_t *pkc_get_pubkey(void *key) > > +{ > > + NvTegraPkcKey *pPkcKey = (NvTegraPkcKey *)key; > > + return (u_int8_t *)pPkcKey->Modulus.Number; > > +} > > pkc_get_pubkey_modulus()? Since presumably *pPkcKey contains more > than > just the Modulus field? > This is because type NvTegraPkcKey is not the same as key type defined by libmcrypto. > > +int > > +pkc_sign_buffer(void *key, u_int8_t *buffer, u_int32_t size, u_int8_t > **pSign) > ... > > + /* TODO: define constant for ssk.len */ > > + ssk.len = (UINT)pPkcKey->Modulus.Digits; /* length in digits of > modulus in term of 32 bits */ > > What is the implication of this TODO? Why isn't this dynamic assignment > acceptable? > Will clean it up. > > @@ -283,17 +321,116 @@ sign_bct(build_image_context *context, > > > - e = sign_data_block(bct + Offset, length, hash_buffer); > > - if (e != 0) > > + if ((e = sign_data_block(bct + Offset, length, hash_buffer)) != 0) > > goto fail; > > The original code was cleaner. > > > - e = g_soc_config->set_data(token_crypto_hash, > > + if ((e = g_soc_config->set_data(token_crypto_hash, > OK. > Same here. It's much better to assign the return code to a variable then > test that variable rather than smash everything together into one > hard-to-read expression. The same comment applies elsewhere too. > > > + /* pkc signing? */ > > + if (context->pkckey) { > ... > > + g_soc_config->set_value(token_pub_key_modulus, > > + pkc_get_pubkey(context->pkckey), > > + bct); > > + } > > + > > + fail: > > + free(hash_buffer); > > + return e; > > +} > > + > > goto fail; > > > > fail: > > free(hash_buffer); > > return e; > > } > > +void > > +SwapEndianness( > > Something is wrong with the indentation at the end of that function, and > there should be a blank line between the functinos. > Will clean it up. > > +int > > +pkc_savefile(const char *pFilename, > > + u_int8_t *buffer, size_t bytes, const char* ext) > > +{ > > + int ret = 0; > > + char *fn = malloc(sizeof(pFilename) + sizeof(ext) + 1); > > sizeof yields the size of the pointer. I suspect you mean strlen. > > > + FILE *output_file; > > + > > + sprintf(fn, "%s%s", pFilename, ext); > > + > > + printf("Saving file %s\n", fn); > > + output_file = fopen(fn, "w+"); > > I assume this is a binary file given the use of fwrite() below. I don't > see any mixture of reading and writing. So that should be "wb". > > > + if (output_file == NULL) { > > + printf("Error opening raw file %s.\n", fn); > > + ret = -1; > > + goto fail; > > + } > > Indentation is wrong there; I suggest scanning all the patches for > TABs-vs-spaces. > > > > +int > > +pkc_save_pubkey( > > + void *pKey, > > + const char *pFileName) > > I think this is saving the modulus and hash, not the key itself? So, > pkc_save_pubkey_modulus_and_hash()? > > > > diff --git a/src/crypto.h b/src/crypto.h > > > +#define PUBKEY_FILENAME "pubkey." > > that shouldn't be hard-coded. > > > +#define PUBKEY_MODULUS "mod" > > +#define PUBKEY_MODULUS_HASH "sha" > > Those names don't imply they're filename extensions. > PUBKEY_FILENAME_EXT_{MODULUS,HASH} would be better. > > > +#define BCT_FILENAME "bct." > > +#define BL_FILENAME "bl." > > FILENAME_PREFIX/SUFFIX? > > > +#define EXT_SIGNATURE "sig" > > FILENAME_EXT_SIGNATURE? > > > +#define NV_BIGINT_MAX_DW 64 > > A comment re: why that's the right max size would be useful. > > > +}NvTegraPkcsVersion; > > +}NvTegraPkcKey; > > Missing space after }. > > > diff --git a/src/data_layout.c b/src/data_layout.c > > > @@ -620,6 +620,24 @@ write_image(build_image_context *context, > file_type image_type) > > token_bl_crypto_hash, > > (u_int32_t*)hash_buffer, > > > + /* save bl sig to file bl.sig */ > > + pkc_savefile(BL_FILENAME, (u_int8_t *)sign, > > + RSA_KEY_BYTE_SIZE, > EXT_SIGNATURE); > > This filename also shouldn't be hard-coded. > OK. > > diff --git a/src/parse.c b/src/parse.c > > > +static int > > +parse_pkckey_file(build_image_context *context, parse_token token, > char *rest) > > > + /* check save option */ > > + if (*rest == ',') { > > + ++rest; > > + > > + /* Parse option. */ > > + if (strstr(rest, OPTION_SAVE) != NULL) > > + save_key = 1; > > This should error out if there's any other unexpected garbage. > > > + } > > else error? > > > diff --git a/src/rsa_key_parse.c b/src/rsa_key_parse.c > > > +static void pkc_string_to_prime(u_int8_t *pBuffer, u_int8_t > *pPrimeNum) > > +{ > > + u_int32_t i = 0; > > + > > + while (i < (RSA_KEY_BYTE_SIZE * 2)) { > > + *pPrimeNum = HEX_TO_DEC(pBuffer[i]) * 16 + > > + HEX_TO_DEC(pBuffer[i + 1]); > > + i += 2; > > + pPrimeNum++; > > + } > > What if the string is too short or has an odd length; are those problems > possible? > > Key length should have been verified before calling this function.. > > +static bool > > +pkc_extract_polar_ssl_primes( > > > + /* Parse POLARSSL format */ > > + pTokenP = (u_int8_t *)strstr((char *)pBuffer, PRIME_PATTERN_P); > > + pTokenQ = (u_int8_t *)strstr((char *)pBuffer, PRIME_PATTERN_Q); > > + > > + if (pTokenP != NULL && pTokenQ != NULL) { > > There would be fewer indentation levels if this was: > > if (!pTokenP || !pTokenQ) > return false; > > > + printf("PKC key in PolarSSL format\n"); > > Shouldn't that be debug output only? The tool shouldn't be noisy all the > time. > > > +static int > > +NvTegraPkcAsnParser( > > + NvU8 *pBuf, > > + NvU32 BufSize, > > + NvU8 *pP, > > + NvU8 *pQ) > > +{ > > + NvS32 i = 0; > > + NvS32 LocalSize = 0; > > + NvU32 Index = 0; > > + NvU32 SequenceNum = 0; > > + NvU32 IntegerNum = 0; > > + NvU32 Expo = 0; > > + NvU8 Type = 0; > > All these initializations just hide used-before-initialized bugs. Can > you please try removing them? The same comment applies elsewhere. > > > + case INTEGER: /* INTEGER */ > > + switch(IntegerNum) > > + { > > + case 0: /* PrivateKeyInfo version */ > > + case 1: /* PrivateKey version */ > > + case 3: /* Modulus */ > ... > > + case 2: /* Public Exponent */ > > All those instances of multiple spaces should be collapsed to just 1. > The same comment may apply elsewhere. > > > + case 4: /* P */ > ... > > + for (i = 0, Index++; i < LocalSize; i++) > > + { > > + *(pP++) = pBuf[Index++]; > > + } > > That looks like memcpy. {} aren't needed. > > > + case 5: /* Q */ > > Cases 4 and 5 are identical, save for whether writing to pP or pQ. Can > they be combined? > > > + default: > > + /* Ideally control should not come here for a .pk8 file */ > > + return NvError_BadParameter; > > "Ideally" implies "should not happen, but we accept it if it happens". > This seems to error out if that happens, which is a strong assertion. > > > +static bool > > +pkc_extract_open_ssl_primes( > > > + if ((memcmp(pBuffer, DerFormat, sizeof(DerFormat)) != 0) && > > + (memcmp(pBuffer, PEMFORMAT_START, > sizeof(PEMFORMAT_START) - 1) != 0)) > > + { > > + return false; > > + } > > Is it guaranteed that in PEM format, there are no blank lines or other > data at the start/end of the file? I don't expect so since that's the > entire point of the large strings that are used as PEM format markers, > but perhaps. > > > + DerKeySize = BufSize - sizeof(PEMFORMAT_END); > > + pPemStart = pBuffer + sizeof(PEMFORMAT_START); > > + > > + if (memcmp(pBuffer + DerKeySize, PEMFORMAT_END, > > + sizeof(PEMFORMAT_END) - 1) != 0) > > + { > > + return false; > > + } > > What if the file is shorter than DerKeySize + strlen(PEMFORMAT_START) + > strlen(PEMFORMAT_END)? > > > + printf("PKC key in Open SSL format\n"); > > + > > + if (memcmp(pBuffer, DerFormat, sizeof(DerFormat)) != 0) > > + { > > Please be consistent about wrapping/not-wrapping braces. I think this > project uses wrapped braces. > > > + DerKeySize = DerKeySize - sizeof(PEMFORMAT_START); > > + > > + e = NvBase64Decode(pPemStart, DerKeySize, NULL, &Base64Size); > > + if (e != NvSuccess) { > > + printf("Base 64 decoding size failed\n"); > > + goto fail; > > + } > > + > > + > > + pBase64Buf = malloc(Base64Size); > > Two blank lines there, and indentation issues throughout the function. > > > +fail: > > + > > + if (pBase64Buf != NULL) > > + free(pBase64Buf); > > free() handles NULL. > > > + if (e != NvSuccess) > > + return false; > > + > > + return true; > > that's: return e == NvSuccess; > > > +static bool > > +pkc_extract_private_keys( > > + u_int8_t *pBuffer, > > + u_int32_t BufSize, > > + u_int8_t *pP, > > + u_int8_t *pQ) > > +{ > > + return > > + ((pkc_extract_open_ssl_primes(pBuffer, BufSize, pP, pQ) == true) || > > + (pkc_extract_polar_ssl_primes(pBuffer, BufSize, pP, pQ) == true)); > > Wonky indentation. No need for "== true"; > > > +static NvU32 NvInverseDigit(NvU32 x) > > +{ > > + NvU32 i = 1; > > + NvU32 j = 2; > > + do > > + { > > + i ^= (j ^ (j & (x * i))); > > + j += j; > > + } > > + while (j); > > "while" should be wrapped with } > Many functions are directly ported from Tegrasign. > What does this function do and how/why? A comment is needed. > > > +static NvU32 > > +NvBigIntIsZero( > > + const NvU32 *x, > > + const NvU32 Digits) > > +{ > > + NvU32 i; > > + for (i = 0; i < Digits; i++) > > + { > > + if (x[i] != 0) > > + { > > + return NV_FALSE; > > + } > > No need for {}. > > > +static NvU32 > > +NvBigIntSubtract( > > + NvU32 *z, > > + const NvU32 *x, > > + const NvU32 *y, > > + const NvU32 Digits) > > +{ > > + NvU32 i, j, k; > > + for (i = k = 0; i < Digits; i ++) > > + { > > + j = x[i] - k; > > + k = (j > (~k)) ? 1 : 0; > > + j -= y[i]; > > + k += (j > (~y[i])) ? 1 : 0; > > + z[i] = j; > > + } > > + return k; > > ">" should evaluate to 1 or 0, so you can drop the ?:. > > It would help to use sane variable names, e.g k -> borrow, j -> tmp or > something like that. > > I would also have expected simpler checks for borrow and only a single > check to be necessary. i.e. a final "if (j > x[i])". I assume that an > NvBigInt is unsigned? NvBigIntcompare's implementation seems to imply so. > > I wonder why all these math functions are re-implemented rather than > making use of the "big digits" code in libmcrypto? Where did all this > math code come from? > > > +static NvU32 NvBigIntGetBit(const NvU32 *x, NvU32 i) > > +{ > > + NvU32 b = 0; > > + > > + return (((x[i >> 5] >> (i & 0x1f)) ^ b) & 1); > > Why "^ b" when b is hard-coded to 0? > > I haven't reviewed the rest of the math functions in any way. > > > +NvBase64Decode( > > > + if (pOutBuf == NULL) > > + { > > + /* Valid if the caller is requesting the size of the decoded > > + * binary buffer so they know how much to alloc. > > + */ > > + *pOutBufSize = 0; > > + } > > + else > > + { > > + /* Validate the size of the passed input data buffer. > > + * In theory the input buffer size should be 3/4 the size of > > + * the decoded buffer. But if there were some white > > + * space chars in input buffer then it is possible that the > > + * input buffer is smaller. > > + * First validate against 2/3rds the size of the input buffer > > + * here. That allows for some slop. > > + * Below code makes sure output buffer size is big enough. > > + */ > > + if (*pOutBufSize < (InBufSize * 2)/3) > > If there's a known maximal minimum size for the buffer, why not just > return that in the !pOutBuf case rather than executing the big/slow loop > below? > > > + /* This loop is less efficient than it could be because > > + * it's designed to tolerate bad characters (like whitespace) > > + * in the input buffer. > > + */ > > + while (i < InBufSize) > > + { > > + NvU8 CurrVal; > > + NvU32 ValidLen = 0; > > + NvU8 ValidBuf[4]; > > + > > + // gather 4 good chars from the pInBufput stream > > Inconsistent comment style. > > > + if (pOutBuf == NULL) > > + { > > + // just measurpInBufg the size of the destpInBufation buffer > > Search/replace typos there. > > > + switch (ValidLen) > > + { > > + case 4: > > + // 4 pInBufput chars equals 3 pOutBufput chars > > + pOutBuf[2] = (unsigned char ) (((ValidBuf[2] << 6) & 0xc0) | > ValidBuf[3]); > > + // fall through > > + case 3: > > + // 3 pInBufput chars equals 2 pOutBufput chars > > + pOutBuf[1] = (unsigned char ) (ValidBuf[1] << 4 | ValidBuf[2] >> > 2); > > + // fall through > > + case 2: > > + // 2 pInBufput chars equals 1 pOutBufput char > > + pOutBuf[0] = (unsigned char ) (ValidBuf[0] << 2 | ValidBuf[1] >> > 4); > > + pOutBuf += ValidLen - 1; > > + break; > > + case 1: > > + // Unexpected > > + break; > > + case 0: > > + // conceivable if white space follows the end of valid data > > + break; > > + default: > > + // Unexpected > > + break; > > + } > > Shouldn't at least cases 1, return an error? > > There are lots of formatting errors in this patch that I didn't > explicitly call out. Lots of cleanup is required. > > I didn't review the math code. It'd be best if it was replaced with > something pre-existing rather than re-inventing the wheel. > > Someone familiar with our chip security needs to review the crypto usage > in this code. > > > diff --git a/src/rsa_key_parse.h b/src/rsa_key_parse.h > > > +#define HEX_TO_DEC(c) \ > > + (((c) >= '0' && (c) <= '9') ? (c) - '0' : \ > > + ((c) >= 'a' && (c) <= 'f') ? (c) - 'a' + 10 : \ > > + ((c) >= 'A' && (c) <= 'F') ? (c) - 'A' + 10 : -1) > > I believe that's already in libmcrypto's hex2byte(). > > > +/* Explicitly sized signed and unsigned ints. */ > > +typedef unsigned char NvU8; /**< 0 to 255 */ > > +typedef unsigned short NvU16; /**< 0 to 65535 */ > > +typedef unsigned int NvU32; /**< 0 to 4294967295 */ > > +typedef unsigned long long NvU64; /**< 0 to 18446744073709551615 */ > > +typedef signed char NvS8; /**< -128 to 127 */ > > +typedef signed short NvS16; /**< -32768 to 32767 */ > > +typedef signed int NvS32; /**< -2147483648 to 2147483647 */ > > +typedef signed long long NvS64; /**< 2^-63 to 2^63-1 */ > > Let's not introduce even more types. cbootimage already has types for > these. > > > +#define NV_TRUE 1 > > +#define NV_FALSE 0 > > Let's just use defines from system header files for these. > > > +#define NvSuccess 0 > > +#define NvError_BadParameter -10 > > +#define NvError_InsufficientMemory -11 > > Can't we use error codes from <errno.h>? > > > +#endif /* #ifndef INCLUDED_RSA_KEY_PARSE_H_N */ > > Just "#endif"; no need for the comment. -- 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
