Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
587014:
Initial code for the TPM Lightweight Command Library. (Closed)
|
Created:
10 years, 10 months ago by Luigi Semenzato Modified:
10 years, 10 months ago Reviewers:
gauravsh CC:
chromium-os-reviews_chromium.org, Will Drewry Visibility:
Public. |
DescriptionInitial code for the TPM Lightweight Command Library.
Patch Set 1 #Patch Set 2 : removed stray file #Patch Set 3 : Changes to comments only. #
Total comments: 12
Patch Set 4 : Eliminated CPP table-driven generation. #Patch Set 5 : Cleanup: mostly naming and comment changes. #
Total comments: 61
Patch Set 6 : Google style conformance. #Patch Set 7 : Renamed various files. #
Total comments: 14
Patch Set 8 : Style changes as per review #
Total comments: 8
Patch Set 9 : Free all allocated memory. Also a few style fixes. #Patch Set 10 : Added missing license #
Messages
Total messages: 7 (0 generated)
I took a quick look. General comments: The API looks very good. Apart from a specific comment (below), it seems sufficient for our needs. I feel macros are a bit overused here and make the code a bit hard to parse in a many places. There's a fair bit of work going on in converting between endianness formats which maybe error prone. I wonder if that could be simplified further. Also, I know you requested I refrain from style comments at this stage but it would make the reading the code a bit easier and less dense if the tlcl_ prefix on function names was dropped. More specific comments below. http://codereview.chromium.org/587014/diff/2010/3004 File src/platform/tpm_lite/src/tlcl/commands.c (right): http://codereview.chromium.org/587014/diff/2010/3004#newcode46 src/platform/tpm_lite/src/tlcl/commands.c:46: * (yay). AFAIK, ARM is bi-endian depending on the mode the processor is running in. I am not sure if this is set during processor reset or at sometime further down in the boot process. We should confirm this. http://codereview.chromium.org/587014/diff/2010/3004#newcode115 src/platform/tpm_lite/src/tlcl/commands.c:115: Could the above be replaced with a single function that reverses the byte ordering? There are also the F* macros at the top. Mainly we care about the direction the data is going to - towards the TPM or from the TPM. http://codereview.chromium.org/587014/diff/2010/3004#newcode144 src/platform/tpm_lite/src/tlcl/commands.c:144: - (2 * PCR_SELECTION_FIX) What's the PCR_SELECTION_FIX for? A comment describing this size calculation would be useful here. http://codereview.chromium.org/587014/diff/2010/3004#newcode161 src/platform/tpm_lite/src/tlcl/commands.c:161: This is confusing - redefining of the macro here. Maybe just call this macro something else (and include commands.def at the top. http://codereview.chromium.org/587014/diff/2010/3004#newcode340 src/platform/tpm_lite/src/tlcl/commands.c:340: } Will this never return an arror? http://codereview.chromium.org/587014/diff/2010/3004#newcode348 src/platform/tpm_lite/src/tlcl/commands.c:348: } Ditto. http://codereview.chromium.org/587014/diff/2010/3008 File src/platform/tpm_lite/src/tlcl/tlcl.h (right): http://codereview.chromium.org/587014/diff/2010/3008#newcode29 src/platform/tpm_lite/src/tlcl/tlcl.h:29: Should we really be ignoring errors here? Under what circumstances can this fail? If, for example, a space with that index already exists, then this might fail. There should be a way to check if an NVRAM region with a given index is already defined - either as an error code from this function or using a separate API call (preferable).
Thanks Gaurav. As discussed, I removed the table-driven preprocessor constructs and addressed most comments. More work is planned on error handling. Also a lot more test cases. At this point I welcome nitpick on style etc. http://codereview.chromium.org/587014/diff/2010/3004 File src/platform/tpm_lite/src/tlcl/commands.c (right): http://codereview.chromium.org/587014/diff/2010/3004#newcode46 src/platform/tpm_lite/src/tlcl/commands.c:46: * (yay). On 2010/02/10 00:10:42, gauravsh wrote: > AFAIK, ARM is bi-endian depending on the mode the processor is running in. I am > not sure if this is set during processor reset or at sometime further down in > the boot process. We should confirm this. ARM endianness can switch at any time (in supervisor mode), but we're using little endian. http://codereview.chromium.org/587014/diff/2010/3004#newcode115 src/platform/tpm_lite/src/tlcl/commands.c:115: On 2010/02/10 00:10:42, gauravsh wrote: > Could the above be replaced with a single function that reverses the byte > ordering? There are also the F* macros at the top. Mainly we care about the > direction the data is going to - towards the TPM or from the TPM. I changed the names to reflect the direction. We could make it a single function, passing the size and the direction, but this seems more readable to me. It's the same thing with fewer function names and more parameters and if statements. http://codereview.chromium.org/587014/diff/2010/3004#newcode144 src/platform/tpm_lite/src/tlcl/commands.c:144: - (2 * PCR_SELECTION_FIX) On 2010/02/10 00:10:42, gauravsh wrote: > What's the PCR_SELECTION_FIX for? A comment describing this size calculation > would be useful here. Yes. Added more comments in the right places. http://codereview.chromium.org/587014/diff/2010/3004#newcode161 src/platform/tpm_lite/src/tlcl/commands.c:161: On 2010/02/10 00:10:42, gauravsh wrote: > This is confusing - redefining of the macro here. Maybe just call this macro > something else (and include commands.def at the top. This is all gone after some discussion on c-users. http://codereview.chromium.org/587014/diff/2010/3004#newcode340 src/platform/tpm_lite/src/tlcl/commands.c:340: } On 2010/02/10 00:10:42, gauravsh wrote: > Will this never return an arror? Yes, it can, even if it would be hard to imagine how. I will revise the entire error handling strategy.
Some initial feedback. http://codereview.chromium.org/587014/diff/7010/7011 File src/platform/tpm_lite/src/testsuite/Makefile (right): http://codereview.chromium.org/587014/diff/7010/7011#newcode9 src/platform/tpm_lite/src/testsuite/Makefile:9: Maybe add a -ansi/-pedantic flag here to make sure this will compile for the firmware as-is. Also, add a -Wall. http://codereview.chromium.org/587014/diff/7010/7011#newcode12 src/platform/tpm_lite/src/testsuite/Makefile:12: Same here. http://codereview.chromium.org/587014/diff/7010/7012 File src/platform/tpm_lite/src/testsuite/readonly.c (right): http://codereview.chromium.org/587014/diff/7010/7012#newcode27 src/platform/tpm_lite/src/testsuite/readonly.c:27: void initialize_spaces(void) { Nit: As per Google-style, InitializeSpaces(). Same goes for all the other functions in this file. http://codereview.chromium.org/587014/diff/7010/7012#newcode34 src/platform/tpm_lite/src/testsuite/readonly.c:34: tlclDefinespace(INDEX1, TPM_NV_PER_WRITE_STCLEAR, 4); Nit: tlclDefineSpace Also, style-guide says this hould be that the 't' should be capitalized, but this looks much cleaner. http://codereview.chromium.org/587014/diff/7010/7012#newcode55 src/platform/tpm_lite/src/testsuite/readonly.c:55: Nit: descriptive variable names, index_0, index_1, etc. http://codereview.chromium.org/587014/diff/7010/7012#newcode87 src/platform/tpm_lite/src/testsuite/readonly.c:87: */ nit: comment necessary? If more stuff needs to go before this, just add a TODO. http://codereview.chromium.org/587014/diff/7010/7013 File src/platform/tpm_lite/src/tlcl/Makefile (right): http://codereview.chromium.org/587014/diff/7010/7013#newcode10 src/platform/tpm_lite/src/tlcl/Makefile:10: Does it make sense to move the .h files to a separate include/ dir? http://codereview.chromium.org/587014/diff/7010/7014 File src/platform/tpm_lite/src/tlcl/commands.c (right): http://codereview.chromium.org/587014/diff/7010/7014#newcode1 src/platform/tpm_lite/src/tlcl/commands.c:1: /* Copyright notice. Same comment for all the other files. http://codereview.chromium.org/587014/diff/7010/7014#newcode21 src/platform/tpm_lite/src/tlcl/commands.c:21: Function Comment? Same for other functions in the file. Also, if any of these are part of the external library interface, they should be declared (with the comment in) the .h file too. http://codereview.chromium.org/587014/diff/7010/7014#newcode21 src/platform/tpm_lite/src/tlcl/commands.c:21: Nit: as per Google-style: TpmTag. Same for other functions too. http://codereview.chromium.org/587014/diff/7010/7014#newcode63 src/platform/tpm_lite/src/tlcl/commands.c:63: static void sendReceive(uint8_t *request, uint8_t *response, int maxlength) { Nit: max_length http://codereview.chromium.org/587014/diff/7010/7014#newcode64 src/platform/tpm_lite/src/tlcl/commands.c:64: uint32_t rlength = maxlength; response_length http://codereview.chromium.org/587014/diff/7010/7014#newcode83 src/platform/tpm_lite/src/tlcl/commands.c:83: static void send(uint8_t *command) { what happens if there's an error? It's unclear from the function name and/or the prototype. http://codereview.chromium.org/587014/diff/7010/7014#newcode151 src/platform/tpm_lite/src/tlcl/commands.c:151: uint32_t rlength; nit: result_length http://codereview.chromium.org/587014/diff/7010/7014#newcode163 src/platform/tpm_lite/src/tlcl/commands.c:163: bcopy(nv_read_cursor, data, rlength); please use memcpy instead. bcopy is marked legacy in POSIX. http://codereview.chromium.org/587014/diff/7010/7015 File src/platform/tpm_lite/src/tlcl/commands.h (right): http://codereview.chromium.org/587014/diff/7010/7015#newcode1 src/platform/tpm_lite/src/tlcl/commands.h:1: #ifndef COMMAND_H TPM_LITE_COMMAND_H_ http://codereview.chromium.org/587014/diff/7010/7015#newcode10 src/platform/tpm_lite/src/tlcl/commands.h:10: const int true = 1; make this const bool? http://codereview.chromium.org/587014/diff/7010/7015#newcode22 src/platform/tpm_lite/src/tlcl/commands.h:22: __attribute__((unused)) What does __attribute__ do? http://codereview.chromium.org/587014/diff/7010/7015#newcode35 src/platform/tpm_lite/src/tlcl/commands.h:35: function comment? http://codereview.chromium.org/587014/diff/7010/7015#newcode43 src/platform/tpm_lite/src/tlcl/commands.h:43: static inline void fromTpmUint32(const uint8_t *buffer, uint32_t *x) { function comment? Same for all the functions below. http://codereview.chromium.org/587014/diff/7010/7015#newcode44 src/platform/tpm_lite/src/tlcl/commands.h:44: *x = (buffer[0] << 24) | Nit: put parentheses around the whole expression. This will also make them line up nicely. http://codereview.chromium.org/587014/diff/7010/7016 File src/platform/tpm_lite/src/tlcl/generator.c (right): http://codereview.chromium.org/587014/diff/7010/7016#newcode5 src/platform/tpm_lite/src/tlcl/generator.c:5: * (packed) TPM structures layout (mostly) match the TPM request and response Is this compiler dependent in anyway? http://codereview.chromium.org/587014/diff/7010/7016#newcode18 src/platform/tpm_lite/src/tlcl/generator.c:18: typedef struct Field { What do various datamembers in Field and Command signify? http://codereview.chromium.org/587014/diff/7010/7016#newcode38 src/platform/tpm_lite/src/tlcl/generator.c:38: static void addVisibleField(Command *c, const char* name, int offset) { Nit: Place * consistently - either next to the type or var name. I have the same problem. :) http://codereview.chromium.org/587014/diff/7010/7016#newcode38 src/platform/tpm_lite/src/tlcl/generator.c:38: static void addVisibleField(Command *c, const char* name, int offset) { Nit: cmd would be a better name (instead of c). http://codereview.chromium.org/587014/diff/7010/7016#newcode68 src/platform/tpm_lite/src/tlcl/generator.c:68: * wrting and reading that field. The fields must be added at increasing This function conflates two different things - initializing a command and adding it to a global linked list. Please separate. http://codereview.chromium.org/587014/diff/7010/7016#newcode71 src/platform/tpm_lite/src/tlcl/generator.c:71: Command* newCommand(TPM_COMMAND_CODE code, int size) { Who owns the returned pointer? What about the malloc()ed field members within the Command structure? http://codereview.chromium.org/587014/diff/7010/7016#newcode77 src/platform/tpm_lite/src/tlcl/generator.c:77: addInitializedField(c, 2, sizeof(uint32_t), size); how is this offset calculated? Based on the the size of the previous initialized field? if yes, use sizeof() instead of hard-coding the values. http://codereview.chromium.org/587014/diff/7010/7016#newcode97 src/platform/tpm_lite/src/tlcl/generator.c:97: * The mismatch occurs in the TPM_PRC_SELECTION structure, and it must be typo: TPM_PCR_SELECTION http://codereview.chromium.org/587014/diff/7010/7016#newcode273 src/platform/tpm_lite/src/tlcl/generator.c:273: outputCommands(Commands); Commands being a global linked list makes the code a bit harder to understand. Can't each of the the build* functions return a Command, which can be explicitly chained to the LL? Commands doesn't need to be a global in that case and the build* functions do not have a side-effect on a global pointer (through newCommand). http://codereview.chromium.org/587014/diff/7010/7016#newcode275 src/platform/tpm_lite/src/tlcl/generator.c:275: Linked list "Commands" is not free-ed. http://codereview.chromium.org/587014/diff/7010/7017 File src/platform/tpm_lite/src/tlcl/structures.h (right): http://codereview.chromium.org/587014/diff/7010/7017#newcode9 src/platform/tpm_lite/src/tlcl/structures.h:9: } tpm_selftestfull_cmd = {{0x0, 0xc1, 0x0, 0x0, 0x0, 0xc, 0x0, 0x0, 0x0, 0x50, }, Since these are constants, shouldn't they just be called kTpmSelf..Cmd etc.? http://codereview.chromium.org/587014/diff/7010/7018 File src/platform/tpm_lite/src/tlcl/tlcl.h (right): http://codereview.chromium.org/587014/diff/7010/7018#newcode14 src/platform/tpm_lite/src/tlcl/tlcl.h:14: * because it runs this command during initialization. I see that the external interface is defined in this file. Shouldn't their implementation be in tlcl.c instead of command.c? http://codereview.chromium.org/587014/diff/7010/7019 File src/platform/tpm_lite/src/tlcl/tpmextras.h (right): http://codereview.chromium.org/587014/diff/7010/7019#newcode13 src/platform/tpm_lite/src/tlcl/tpmextras.h:13: typedef struct tdTPM_WRITE_INFO { -td
Well, that took a while. Thanks. I think I addressed all the issues, but there were a lot of them, so I probably missed a few. I think it's ready for review. I have to go to the airport now. If I had more time I'd do another pass, but I'd probably still miss a few things, so it may be OK to review it now. Unfortunately there's no internet where I am going. See you on Wednesday. http://codereview.chromium.org/587014/diff/7010/7011 File src/platform/tpm_lite/src/testsuite/Makefile (right): http://codereview.chromium.org/587014/diff/7010/7011#newcode12 src/platform/tpm_lite/src/testsuite/Makefile:12: On 2010/02/12 01:25:38, gauravsh wrote: > Same here. Thanks. I added all the flags (to the compilation phase, not the linking). I had to change one comment from // to /* */. http://codereview.chromium.org/587014/diff/7010/7012 File src/platform/tpm_lite/src/testsuite/readonly.c (right): http://codereview.chromium.org/587014/diff/7010/7012#newcode27 src/platform/tpm_lite/src/testsuite/readonly.c:27: void initialize_spaces(void) { On 2010/02/12 01:25:38, gauravsh wrote: > Nit: As per Google-style, InitializeSpaces(). Same goes for all the other > functions in this file. Yes, thanks. I am fixing everything. (Boring.) http://codereview.chromium.org/587014/diff/7010/7012#newcode34 src/platform/tpm_lite/src/testsuite/readonly.c:34: tlclDefinespace(INDEX1, TPM_NV_PER_WRITE_STCLEAR, 4); On 2010/02/12 01:25:38, gauravsh wrote: > Nit: tlclDefineSpace > > Also, style-guide says this hould be that the 't' should be capitalized, but > this looks much cleaner. Yes, I agree with you. However it doesn't look that much uglier as TlclDefineSpace, so I will go by the rules. http://codereview.chromium.org/587014/diff/7010/7012#newcode55 src/platform/tpm_lite/src/testsuite/readonly.c:55: On 2010/02/12 01:25:38, gauravsh wrote: > Nit: descriptive variable names, index_0, index_1, etc. Yes! http://codereview.chromium.org/587014/diff/7010/7013 File src/platform/tpm_lite/src/tlcl/Makefile (right): http://codereview.chromium.org/587014/diff/7010/7013#newcode10 src/platform/tpm_lite/src/tlcl/Makefile:10: On 2010/02/12 01:25:38, gauravsh wrote: > Does it make sense to move the .h files to a separate include/ dir? Sure. It makes it easier to see what's exported. http://codereview.chromium.org/587014/diff/7010/7014 File src/platform/tpm_lite/src/tlcl/commands.c (right): http://codereview.chromium.org/587014/diff/7010/7014#newcode83 src/platform/tpm_lite/src/tlcl/commands.c:83: static void send(uint8_t *command) { On 2010/02/12 01:25:38, gauravsh wrote: > what happens if there's an error? It's unclear from the function name and/or the > prototype. I clarified with comments that error checking here is only meaningful from user mode, for debugging and testing. The complete error recovery strategy in the firmware needs more work. http://codereview.chromium.org/587014/diff/7010/7015 File src/platform/tpm_lite/src/tlcl/commands.h (right): http://codereview.chromium.org/587014/diff/7010/7015#newcode1 src/platform/tpm_lite/src/tlcl/commands.h:1: #ifndef COMMAND_H On 2010/02/12 01:25:38, gauravsh wrote: > TPM_LITE_COMMAND_H_ > How about tlcl_internal.h. http://codereview.chromium.org/587014/diff/7010/7015#newcode10 src/platform/tpm_lite/src/tlcl/commands.h:10: const int true = 1; On 2010/02/12 01:25:38, gauravsh wrote: > make this const bool? Yep! http://codereview.chromium.org/587014/diff/7010/7015#newcode22 src/platform/tpm_lite/src/tlcl/commands.h:22: __attribute__((unused)) On 2010/02/12 01:25:38, gauravsh wrote: > What does __attribute__ do? __attribute__((unused)) prevents gcc from emitting warnings (or errors) when a static function is not called. We'll have to deal with this on a compiler-by-compiler basis. http://codereview.chromium.org/587014/diff/7010/7015#newcode44 src/platform/tpm_lite/src/tlcl/commands.h:44: *x = (buffer[0] << 24) | On 2010/02/12 01:25:38, gauravsh wrote: > Nit: put parentheses around the whole expression. This will also make them line > up nicely. I like that! http://codereview.chromium.org/587014/diff/7010/7016 File src/platform/tpm_lite/src/tlcl/generator.c (right): http://codereview.chromium.org/587014/diff/7010/7016#newcode5 src/platform/tpm_lite/src/tlcl/generator.c:5: * (packed) TPM structures layout (mostly) match the TPM request and response On 2010/02/12 01:25:38, gauravsh wrote: > Is this compiler dependent in anyway? Yes, but it's the best we can do at this point. Also note that only the build-time code needs to be compiled with -fpack-struct, so as long as we're building this on Linux (which we are) we'll be fine. http://codereview.chromium.org/587014/diff/7010/7016#newcode18 src/platform/tpm_lite/src/tlcl/generator.c:18: typedef struct Field { On 2010/02/12 01:25:38, gauravsh wrote: > What do various datamembers in Field and Command signify? Adding comments. http://codereview.chromium.org/587014/diff/7010/7016#newcode38 src/platform/tpm_lite/src/tlcl/generator.c:38: static void addVisibleField(Command *c, const char* name, int offset) { On 2010/02/12 01:25:38, gauravsh wrote: > Nit: cmd would be a better name (instead of c). Really? I changed them, but the single-character variable stood out more. I agree that for longer functions a longer name is better, for textual search/replace operations. (Although \<c\> worked fine.) http://codereview.chromium.org/587014/diff/7010/7016#newcode68 src/platform/tpm_lite/src/tlcl/generator.c:68: * wrting and reading that field. The fields must be added at increasing On 2010/02/12 01:25:38, gauravsh wrote: > This function conflates two different things - initializing a command and adding > it to a global linked list. Please separate. I unconflated it! http://codereview.chromium.org/587014/diff/7010/7016#newcode71 src/platform/tpm_lite/src/tlcl/generator.c:71: Command* newCommand(TPM_COMMAND_CODE code, int size) { On 2010/02/12 01:25:38, gauravsh wrote: > Who owns the returned pointer? What about the malloc()ed field members within > the Command structure? This shouldn't be an issue, because this program is not freeing anything. Do we really have to worry about memory allocation rules in programs that will never free anything? Do we have to free memory when it is clear that a program will never need to free it? Maybe I could make this clearer by using my own version of malloc, which allocates from a 10k static array. http://codereview.chromium.org/587014/diff/7010/7016#newcode77 src/platform/tpm_lite/src/tlcl/generator.c:77: addInitializedField(c, 2, sizeof(uint32_t), size); On 2010/02/12 01:25:38, gauravsh wrote: > how is this offset calculated? Based on the the size of the previous initialized > field? if yes, use sizeof() instead of hard-coding the values. Sure. http://codereview.chromium.org/587014/diff/7010/7016#newcode273 src/platform/tpm_lite/src/tlcl/generator.c:273: outputCommands(Commands); On 2010/02/12 01:25:38, gauravsh wrote: > Commands being a global linked list makes the code a bit harder to understand. > Can't each of the the build* functions return a Command, which can be explicitly > chained to the LL? Commands doesn't need to be a global in that case and the > build* functions do not have a side-effect on a global pointer (through > newCommand). OK http://codereview.chromium.org/587014/diff/7010/7016#newcode275 src/platform/tpm_lite/src/tlcl/generator.c:275: On 2010/02/12 01:25:38, gauravsh wrote: > Linked list "Commands" is not free-ed. Nope. This program doesn't free. http://codereview.chromium.org/587014/diff/7010/7017 File src/platform/tpm_lite/src/tlcl/structures.h (right): http://codereview.chromium.org/587014/diff/7010/7017#newcode9 src/platform/tpm_lite/src/tlcl/structures.h:9: } tpm_selftestfull_cmd = {{0x0, 0xc1, 0x0, 0x0, 0x0, 0xc, 0x0, 0x0, 0x0, 0x50, }, On 2010/02/12 01:25:38, gauravsh wrote: > Since these are constants, shouldn't they just be called kTpmSelf..Cmd etc.? They are not completely constant---some of their fields are modified at run time. http://codereview.chromium.org/587014/diff/7010/7018 File src/platform/tpm_lite/src/tlcl/tlcl.h (right): http://codereview.chromium.org/587014/diff/7010/7018#newcode14 src/platform/tpm_lite/src/tlcl/tlcl.h:14: * because it runs this command during initialization. On 2010/02/12 01:25:38, gauravsh wrote: > I see that the external interface is defined in this file. Shouldn't their > implementation be in tlcl.c instead of command.c? Yes. http://codereview.chromium.org/587014/diff/7010/7019 File src/platform/tpm_lite/src/tlcl/tpmextras.h (right): http://codereview.chromium.org/587014/diff/7010/7019#newcode13 src/platform/tpm_lite/src/tlcl/tpmextras.h:13: typedef struct tdTPM_WRITE_INFO { On 2010/02/12 01:25:38, gauravsh wrote: > -td Actually, that's the convention in /usr/include/tss/tpm.h. Since I am mimicking that, I prefer to follow that convention.
Looking good. Specific comments below. A general request for style consistency
in the firmware code - can we agree on the C-style comments we'll be using?
I am using: /* xxxxx
* xxxxxx
*/
and sometime /* xxxxx */ for one line comments.
You use: /*
* xxxx
*/
I prefer my style since it takes up less vertical white- space. In any case, we
should try and converge on one. :)
http://codereview.chromium.org/587014/diff/7010/7012
File src/platform/tpm_lite/src/testsuite/readonly.c (right):
http://codereview.chromium.org/587014/diff/7010/7012#newcode87
src/platform/tpm_lite/src/testsuite/readonly.c:87: */
On 2010/02/12 01:25:38, gauravsh wrote:
> nit: comment necessary? If more stuff needs to go before this, just add a
TODO.
http://codereview.chromium.org/587014/diff/7010/7015
File src/platform/tpm_lite/src/tlcl/commands.h (right):
http://codereview.chromium.org/587014/diff/7010/7015#newcode1
src/platform/tpm_lite/src/tlcl/commands.h:1: #ifndef COMMAND_H
On 2010/02/12 19:42:46, Luigi Semenzato wrote:
> On 2010/02/12 01:25:38, gauravsh wrote:
> > TPM_LITE_COMMAND_H_
> >
>
> How about tlcl_internal.h.
>
That works. However, the #ifndef blocks should always prefix the name of the
module (TPM_LITE_)
http://codereview.chromium.org/587014/diff/7010/7016
File src/platform/tpm_lite/src/tlcl/generator.c (right):
http://codereview.chromium.org/587014/diff/7010/7016#newcode38
src/platform/tpm_lite/src/tlcl/generator.c:38: static void
addVisibleField(Command *c, const char* name, int offset) {
On 2010/02/12 19:42:46, Luigi Semenzato wrote:
> On 2010/02/12 01:25:38, gauravsh wrote:
> > Nit: cmd would be a better name (instead of c).
>
> Really? I changed them, but the single-character variable stood out more. I
> agree that for longer functions a longer name is better, for textual
> search/replace operations. (Although \<c\> worked fine.)
>
The reason I asked to change it to cmd is that 'c' is usually idiomatic for
character variables (much like 'i' is for integers).
http://codereview.chromium.org/587014/diff/7010/7016#newcode71
src/platform/tpm_lite/src/tlcl/generator.c:71: Command*
newCommand(TPM_COMMAND_CODE code, int size) {
On 2010/02/12 19:42:46, Luigi Semenzato wrote:
> On 2010/02/12 01:25:38, gauravsh wrote:
> > Who owns the returned pointer? What about the malloc()ed field members
within
> > the Command structure?
>
> This shouldn't be an issue, because this program is not freeing anything.
>
> Do we really have to worry about memory allocation rules in programs that will
> never free anything?
>
> Do we have to free memory when it is clear that a program will never need to
> free it?
>
> Maybe I could make this clearer by using my own version of malloc, which
> allocates from a 10k static array.
>
>
The way I am handling this issue is to define function stubs for Malloc() and
Free() which right now just call the linux version. This can later be linked to
use whatever we do in firmware land. See the common/ directory under
src/platform/vboot_reference.
http://codereview.chromium.org/587014/diff/7010/7016#newcode275
src/platform/tpm_lite/src/tlcl/generator.c:275:
On 2010/02/12 19:42:46, Luigi Semenzato wrote:
> On 2010/02/12 01:25:38, gauravsh wrote:
> > Linked list "Commands" is not free-ed.
>
> Nope. This program doesn't free.
As per my comment above, maybe just call a Free() wrapper. We can change it
whatever we want later (or just a no-op).
http://codereview.chromium.org/587014/diff/7010/7019
File src/platform/tpm_lite/src/tlcl/tpmextras.h (right):
http://codereview.chromium.org/587014/diff/7010/7019#newcode13
src/platform/tpm_lite/src/tlcl/tpmextras.h:13: typedef struct tdTPM_WRITE_INFO {
On 2010/02/12 19:42:46, Luigi Semenzato wrote:
> On 2010/02/12 01:25:38, gauravsh wrote:
> > -td
>
> Actually, that's the convention in /usr/include/tss/tpm.h. Since I am
mimicking
> that, I prefer to follow that convention.
Sounds good.
http://codereview.chromium.org/587014/diff/5002/1010
File src/platform/tpm_lite/src/include/tlcl.h (right):
http://codereview.chromium.org/587014/diff/5002/1010#newcode12
src/platform/tpm_lite/src/include/tlcl.h:12: #ifndef TLCL_H
Nit: TPM_LITE_TLCL_H_
http://codereview.chromium.org/587014/diff/5002/1010#newcode20
src/platform/tpm_lite/src/include/tlcl.h:20: void TlclLibinit(void);
Nit: TlclLibInit
http://codereview.chromium.org/587014/diff/5002/1010#newcode32
src/platform/tpm_lite/src/include/tlcl.h:32: void TlclSelftestfull(void);
Nit: TlclSelfTestFull
http://codereview.chromium.org/587014/diff/5002/1012
File src/platform/tpm_lite/src/testsuite/readonly.c (right):
http://codereview.chromium.org/587014/diff/5002/1012#newcode8
src/platform/tpm_lite/src/testsuite/readonly.c:8: * TPM functionality.
Since this is a test, make it clear in the comments that this checks for
initialization of NVRAM spaces.
http://codereview.chromium.org/587014/diff/5002/1012#newcode88
src/platform/tpm_lite/src/testsuite/readonly.c:88: if (TlclRead(INDEX0, (uint8_t
*) &index_0, 4) != TPM_SUCCESS ||
s/4/sizeof(index_0)
or
s/4/sizeof(uint32_t)
http://codereview.chromium.org/587014/diff/5002/1014
File src/platform/tpm_lite/src/tlcl/generator.c (right):
http://codereview.chromium.org/587014/diff/5002/1014#newcode25
src/platform/tpm_lite/src/tlcl/generator.c:25: * command. NAME is the field
name. VISIBLE is true if the field is modified
I think Google-style is to distinguish varnames in comments by using | or [] -
e.g. [name] |name|. I could be wrong about this though. Please check.
http://codereview.chromium.org/587014/diff/5002/1014#newcode42
src/platform/tpm_lite/src/tlcl/generator.c:42: * of a TPM command. SIZE is the
size of the command buffer in bytes, when
Same comment as above.
http://codereview.chromium.org/587014/diff/5002/1014#newcode160
src/platform/tpm_lite/src/tlcl/generator.c:160: * BuildXXX builds TPM command
XXX.
Probably just one of these comments at the beginning is fine. :)
http://codereview.chromium.org/587014/diff/5002/1014#newcode320
src/platform/tpm_lite/src/tlcl/generator.c:320: Command *cmd = builders[i]();
Command* cmd
http://codereview.chromium.org/587014/diff/5002/1015
File src/platform/tpm_lite/src/tlcl/tlcl.c (right):
http://codereview.chromium.org/587014/diff/5002/1015#newcode1
src/platform/tpm_lite/src/tlcl/tlcl.c:1: /*
Copyright notice
http://codereview.chromium.org/587014/diff/5002/1015#newcode79
src/platform/tpm_lite/src/tlcl/tlcl.c:79: int tag, rtag;
Nit: response_tag
http://codereview.chromium.org/587014/diff/5002/1015#newcode110
src/platform/tpm_lite/src/tlcl/tlcl.c:110: * Library initialization.
Comments for exported functions (declared in tlcl.h) shouldn't be repeated here.
http://codereview.chromium.org/587014/diff/5002/1016
File src/platform/tpm_lite/src/tlcl/tlcl_internal.h (right):
http://codereview.chromium.org/587014/diff/5002/1016#newcode1
src/platform/tpm_lite/src/tlcl/tlcl_internal.h:1: #ifndef TLCL_INTERNAL_H
Copyright Notice.
TPM_LITE_TLCL_INTERNAL_H_
http://codereview.chromium.org/587014/diff/5002/1017
File src/platform/tpm_lite/src/tlcl/tpmextras.h (right):
http://codereview.chromium.org/587014/diff/5002/1017#newcode10
src/platform/tpm_lite/src/tlcl/tpmextras.h:10: #ifndef TPMEXTRAS_H
TPM_LITE_TPMEXTRAS_H_
Thanks. Fixed everything. On 2010/02/12 20:42:07, gauravsh wrote: > Looking good. Specific comments below. A general request for style consistency > in the firmware code - can we agree on the C-style comments we'll be using? > > I am using: /* xxxxx > * xxxxxx > */ > and sometime /* xxxxx */ for one line comments. > > You use: /* > * xxxx > */ > > I prefer my style since it takes up less vertical white- space. In any case, we > should try and converge on one. :) > > http://codereview.chromium.org/587014/diff/7010/7012 > File src/platform/tpm_lite/src/testsuite/readonly.c (right): > > http://codereview.chromium.org/587014/diff/7010/7012#newcode87 > src/platform/tpm_lite/src/testsuite/readonly.c:87: */ > On 2010/02/12 01:25:38, gauravsh wrote: > > nit: comment necessary? If more stuff needs to go before this, just add a > TODO. > > http://codereview.chromium.org/587014/diff/7010/7015 > File src/platform/tpm_lite/src/tlcl/commands.h (right): > > http://codereview.chromium.org/587014/diff/7010/7015#newcode1 > src/platform/tpm_lite/src/tlcl/commands.h:1: #ifndef COMMAND_H > On 2010/02/12 19:42:46, Luigi Semenzato wrote: > > On 2010/02/12 01:25:38, gauravsh wrote: > > > TPM_LITE_COMMAND_H_ > > > > > > > How about tlcl_internal.h. > > > That works. However, the #ifndef blocks should always prefix the name of the > module (TPM_LITE_) > > http://codereview.chromium.org/587014/diff/7010/7016 > File src/platform/tpm_lite/src/tlcl/generator.c (right): > > http://codereview.chromium.org/587014/diff/7010/7016#newcode38 > src/platform/tpm_lite/src/tlcl/generator.c:38: static void > addVisibleField(Command *c, const char* name, int offset) { > On 2010/02/12 19:42:46, Luigi Semenzato wrote: > > On 2010/02/12 01:25:38, gauravsh wrote: > > > Nit: cmd would be a better name (instead of c). > > > > Really? I changed them, but the single-character variable stood out more. I > > agree that for longer functions a longer name is better, for textual > > search/replace operations. (Although \<c\> worked fine.) > > > > The reason I asked to change it to cmd is that 'c' is usually idiomatic for > character variables (much like 'i' is for integers). > > http://codereview.chromium.org/587014/diff/7010/7016#newcode71 > src/platform/tpm_lite/src/tlcl/generator.c:71: Command* > newCommand(TPM_COMMAND_CODE code, int size) { > On 2010/02/12 19:42:46, Luigi Semenzato wrote: > > On 2010/02/12 01:25:38, gauravsh wrote: > > > Who owns the returned pointer? What about the malloc()ed field members > within > > > the Command structure? > > > > This shouldn't be an issue, because this program is not freeing anything. > > > > Do we really have to worry about memory allocation rules in programs that will > > never free anything? > > > > Do we have to free memory when it is clear that a program will never need to > > free it? > > > > Maybe I could make this clearer by using my own version of malloc, which > > allocates from a 10k static array. > > > > > > The way I am handling this issue is to define function stubs for Malloc() and > Free() which right now just call the linux version. This can later be linked to > use whatever we do in firmware land. See the common/ directory under > src/platform/vboot_reference. > > http://codereview.chromium.org/587014/diff/7010/7016#newcode275 > src/platform/tpm_lite/src/tlcl/generator.c:275: > On 2010/02/12 19:42:46, Luigi Semenzato wrote: > > On 2010/02/12 01:25:38, gauravsh wrote: > > > Linked list "Commands" is not free-ed. > > > > Nope. This program doesn't free. > As per my comment above, maybe just call a Free() wrapper. We can change it > whatever we want later (or just a no-op). > > http://codereview.chromium.org/587014/diff/7010/7019 > File src/platform/tpm_lite/src/tlcl/tpmextras.h (right): > > http://codereview.chromium.org/587014/diff/7010/7019#newcode13 > src/platform/tpm_lite/src/tlcl/tpmextras.h:13: typedef struct tdTPM_WRITE_INFO { > On 2010/02/12 19:42:46, Luigi Semenzato wrote: > > On 2010/02/12 01:25:38, gauravsh wrote: > > > -td > > > > Actually, that's the convention in /usr/include/tss/tpm.h. Since I am > mimicking > > that, I prefer to follow that convention. > > Sounds good. > > http://codereview.chromium.org/587014/diff/5002/1010 > File src/platform/tpm_lite/src/include/tlcl.h (right): > > http://codereview.chromium.org/587014/diff/5002/1010#newcode12 > src/platform/tpm_lite/src/include/tlcl.h:12: #ifndef TLCL_H > Nit: TPM_LITE_TLCL_H_ > > http://codereview.chromium.org/587014/diff/5002/1010#newcode20 > src/platform/tpm_lite/src/include/tlcl.h:20: void TlclLibinit(void); > Nit: TlclLibInit > > http://codereview.chromium.org/587014/diff/5002/1010#newcode32 > src/platform/tpm_lite/src/include/tlcl.h:32: void TlclSelftestfull(void); > Nit: TlclSelfTestFull > > http://codereview.chromium.org/587014/diff/5002/1012 > File src/platform/tpm_lite/src/testsuite/readonly.c (right): > > http://codereview.chromium.org/587014/diff/5002/1012#newcode8 > src/platform/tpm_lite/src/testsuite/readonly.c:8: * TPM functionality. > Since this is a test, make it clear in the comments that this checks for > initialization of NVRAM spaces. > > http://codereview.chromium.org/587014/diff/5002/1012#newcode88 > src/platform/tpm_lite/src/testsuite/readonly.c:88: if (TlclRead(INDEX0, (uint8_t > *) &index_0, 4) != TPM_SUCCESS || > s/4/sizeof(index_0) > > or > > s/4/sizeof(uint32_t) > > http://codereview.chromium.org/587014/diff/5002/1014 > File src/platform/tpm_lite/src/tlcl/generator.c (right): > > http://codereview.chromium.org/587014/diff/5002/1014#newcode25 > src/platform/tpm_lite/src/tlcl/generator.c:25: * command. NAME is the field > name. VISIBLE is true if the field is modified > I think Google-style is to distinguish varnames in comments by using | or [] - > e.g. [name] |name|. I could be wrong about this though. Please check. > > http://codereview.chromium.org/587014/diff/5002/1014#newcode42 > src/platform/tpm_lite/src/tlcl/generator.c:42: * of a TPM command. SIZE is the > size of the command buffer in bytes, when > Same comment as above. > > http://codereview.chromium.org/587014/diff/5002/1014#newcode160 > src/platform/tpm_lite/src/tlcl/generator.c:160: * BuildXXX builds TPM command > XXX. > Probably just one of these comments at the beginning is fine. :) > > http://codereview.chromium.org/587014/diff/5002/1014#newcode320 > src/platform/tpm_lite/src/tlcl/generator.c:320: Command *cmd = builders[i](); > Command* cmd > > http://codereview.chromium.org/587014/diff/5002/1015 > File src/platform/tpm_lite/src/tlcl/tlcl.c (right): > > http://codereview.chromium.org/587014/diff/5002/1015#newcode1 > src/platform/tpm_lite/src/tlcl/tlcl.c:1: /* > Copyright notice > > http://codereview.chromium.org/587014/diff/5002/1015#newcode79 > src/platform/tpm_lite/src/tlcl/tlcl.c:79: int tag, rtag; > Nit: response_tag > > http://codereview.chromium.org/587014/diff/5002/1015#newcode110 > src/platform/tpm_lite/src/tlcl/tlcl.c:110: * Library initialization. > Comments for exported functions (declared in tlcl.h) shouldn't be repeated here. > > http://codereview.chromium.org/587014/diff/5002/1016 > File src/platform/tpm_lite/src/tlcl/tlcl_internal.h (right): > > http://codereview.chromium.org/587014/diff/5002/1016#newcode1 > src/platform/tpm_lite/src/tlcl/tlcl_internal.h:1: #ifndef TLCL_INTERNAL_H > Copyright Notice. > TPM_LITE_TLCL_INTERNAL_H_ > > http://codereview.chromium.org/587014/diff/5002/1017 > File src/platform/tpm_lite/src/tlcl/tpmextras.h (right): > > http://codereview.chromium.org/587014/diff/5002/1017#newcode10 > src/platform/tpm_lite/src/tlcl/tpmextras.h:10: #ifndef TPMEXTRAS_H > TPM_LITE_TPMEXTRAS_H_
LGTM after misc fixes below. http://codereview.chromium.org/587014/diff/13001/13002 File src/platform/tpm_lite/src/include/tlcl.h (right): http://codereview.chromium.org/587014/diff/13001/13002#newcode30 src/platform/tpm_lite/src/include/tlcl.h:30: /* Defines a space with permission PERM. INDEX is the index for the space, Nit: arguments specified with their actual name within []. Same for all other comments in this file. http://codereview.chromium.org/587014/diff/13001/13003 File src/platform/tpm_lite/src/testsuite/Makefile (right): http://codereview.chromium.org/587014/diff/13001/13003#newcode9 src/platform/tpm_lite/src/testsuite/Makefile:9: cc -g readonly.o -o readonly \ You might want to replace cc with a conditional variable CC. This will help with the cross-compilation when the library is built as part of a test. CC ?= cc http://codereview.chromium.org/587014/diff/13001/13005 File src/platform/tpm_lite/src/tlcl/Makefile (right): http://codereview.chromium.org/587014/diff/13001/13005#newcode18 src/platform/tpm_lite/src/tlcl/Makefile:18: s/cc/$(CC)/ http://codereview.chromium.org/587014/diff/13001/13006 File src/platform/tpm_lite/src/tlcl/generator.c (right): http://codereview.chromium.org/587014/diff/13001/13006#newcode55 src/platform/tpm_lite/src/tlcl/generator.c:55: Field* fld = (Field*) malloc(sizeof(Field)); either stub this to a helper Malloc() which calls malloc(). Or call free() explicitly after the call to OutputCommands() in main. http://codereview.chromium.org/587014/diff/13001/13007 File src/platform/tpm_lite/src/tlcl/tlcl.c (right): http://codereview.chromium.org/587014/diff/13001/13007#newcode72 src/platform/tpm_lite/src/tlcl/tlcl.c:72: /* Sends a request and receive a response. Nit: description of parameters? http://codereview.chromium.org/587014/diff/13001/13008 File src/platform/tpm_lite/src/tlcl/tlcl_internal.h (right): http://codereview.chromium.org/587014/diff/13001/13008#newcode1 src/platform/tpm_lite/src/tlcl/tlcl_internal.h:1: #ifndef TPM_LITE_TLCL_INTERNAL_H_ Copyright header http://codereview.chromium.org/587014/diff/13001/13008#newcode22 src/platform/tpm_lite/src/tlcl/tlcl_internal.h:22: * Output an error message and quit the program. Nit: Comment style in the whole file. http://codereview.chromium.org/587014/diff/13001/13009 File src/platform/tpm_lite/src/tlcl/tpmextras.h (right): http://codereview.chromium.org/587014/diff/13001/13009#newcode7 src/platform/tpm_lite/src/tlcl/tpmextras.h:7: * TPM definitions not available in any TSS include file :-( Nit: comment style. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
