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
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")?
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?
diff --git a/src/cbootimage.h b/src/cbootimage.h
+typedef enum
+{
+ false = 0,
+ true = 1,
+} 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).
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?
+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?
@@ -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,
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.
+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.
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?
+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 }
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
