Add a cli command to test the TPM device.
PATCH v2 2/2 Add a cli command to test the TPM device.
The command gets an arbitrary number of arguments (up to 30), which are interpreted as byte values and are feed into the TPM device after proper initialization. Then the return value and data of the TPM driver is examined.
TPM commands are described in the TCG specification.
For instance, the following sequence is the 'TPM Startup' command, it is processed by the TPM and a response is generated:
boot > tpm 0x0 0xc1 0x0 0x0 0x0 0xc 0x0 0x0 0x0 0x99 0x0 0x1 Found TPM SLB9635 TT 1.2 by Infineon
Got TPM response:
00 c4 00 00 00 0a 00 00 00 00
If the command is corrupted (fed one byte short), an error is reported:
boot > tpm 0x0 0xc1 0x0 0x0 0x0 0xc 0x0 0x0 0x0 0x99 0x0 generic_lpc_tpm.c:311 unexpected TPM status 0xff000888 generic_lpc_tpm.c:516 failed sending data to TPM
tpm command failed
boot >
--- common/Makefile | 2 + common/cmd_tpm.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 0 deletions(-) create mode 100644 common/cmd_tpm.c
common/Makefile +2 -0
@@ -153,6 +153,8 @@ COBJS-$(CONFIG_CMD_UBI) += cmd_ubi.o COBJS-$(CONFIG_CMD_UBIFS) += cmd_ubifs.o COBJS-$(CONFIG_CMD_UNIVERSE) += cmd_universe.o COBJS-$(CONFIG_CMD_UNZIP) += cmd_unzip.o +COBJS-$(CONFIG_CMD_TPM) += cmd_tpm.o + ifdef CONFIG_CMD_USB COBJS-y += cmd_usb.o COBJS-y += usb.o
common/cmd_tpm.c new file +111 -0
111 lines not on this page · raw message
Signature · 1 line
1.7.3.1
PATCH v2 2/2 Add a cli command to test the TPM device.
Quoted 5 lines · Vadim Bendebury
The command gets an arbitrary number of arguments (up to 30), which are interpreted as byte values and are feed into the TPM device after proper initialization. Then the return value and data of the TPM driver is examined.
Dear Vadim Bendebury,
[...]
common/cmd_tpm.c line 3
Quoted 9 lines
common/cmd_tpm.c new file +3
-0@@ -0,0 +1,111 @@ +/* +* Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +* Released under the 2-clause BSD license.
Are we ok with this ? Also, you say something about GPL in the same comment?
Quoted 20 lines
common/cmd_tpm.c new file +20
-0+* +* See file CREDITS for list of people who contributed to this +* project. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; either version 2 of +* the License, or (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, +* MA 02111-1307 USA +*/ +
[...]
common/cmd_tpm.c line 66
Quoted 10 lines
common/cmd_tpm.c new file +10
-0+puts ("Got TPM response:\n" ); +for (i =0 ; i < read_size; i++) +printf (" %2.2x" , tpm_buffer[i]); +puts ("\n" ); + rv =0 ; + }else { +puts ("tpm command failed\n" ); + } +return rv; +}
You might want to use debug() where fitting ?
common/cmd_tpm.c line 78
Quoted 12 lines
common/cmd_tpm.c new file +2
-0+ +static int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * constargv[]) +{
common/cmd_tpm.c new file +9
-0+int rv =0 ; + +/* +* Verify that in case it is present, the first argument, it is +* exactly one character in size. +*/ +if (argc <7 ) { +puts ("command should be at least six bytes in size\n" ); +return ~0 ;
Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
Quoted 7 lines
common/cmd_tpm.c new file +7
-0+ } + +if (tis_init ()) { +puts ("tis_init() failed!\n" ); +return ~0 ; + } +
common/cmd_tpm.c line 95
Quoted 10 lines
common/cmd_tpm.c new file +10
-0+if (tis_open ()) { +puts ("tis_open() failed!\n" ); +return ~0 ; + } + + rv =tpm_process (argc -1 , argv +1 ); + +if (!rv &&tis_close ()) { +puts ("tis_close() failed!\n" ); + rv = ~0 ;
This doesn't make much sense, tis_close() might not be called under all circumstances, is it ok ?
common/cmd_tpm.c line 108
Quoted 13 lines
common/cmd_tpm.c new file +13
-0+ } + +return rv; +} + +U_BOOT_CMD (tpm,MAX_TRANSACTION_SIZE ,1 , do_tpm, +"tpm <data> [<data>] - write data and read ressponse\n" , +"send arbitrary data to the TPM and read the response" +); + +static void report_error (const char *msg) +{ +if (msg && *msg)
Uhm, you also check if first character is non-zero? why ?
common/cmd_tpm.c line 110
Quoted 2 lines
common/cmd_tpm.c new file +2
-0+printf ("%s\n" , msg); +cmd_usage (&__u_boot_cmd_tpm);
Two underscores aren't a good practice.
Quoted 1 line
+}
Cheers
PATCH v2 2/2 Add a cli command to test the TPM device.
Dear Marek Vasut,
thank you for your comments, please see below:
Quoted 11 lines · Marek Vasut
Quoted 5 lines · Vadim Bendebury
The command gets an arbitrary number of arguments (up to 30), which are interpreted as byte values and are feed into the TPM device after proper initialization. Then the return value and data of the TPM driver is examined.
Dear Vadim Bendebury,
[...]
Quoted 12 lines
common/cmd_tpm.c line 3
Quoted 9 lines
common/cmd_tpm.c new file +3
-0@@ -0,0 +1,111 @@ +/* +* Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +* Released under the 2-clause BSD license. Are we ok with this ? Also, you say something about GPL in the same comment?
Can someone please tell me what needs to be put in the license headers and I will do it. I hear different suggestions from different people.
Quoted 34 lines
Quoted 20 lines
common/cmd_tpm.c new file +20
-0+* +* See file CREDITS for list of people who contributed to this +* project. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; either version 2 of +* the License, or (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, +* MA 02111-1307 USA +*/ +[...]
Quoted 10 lines
+ ? ? ? ? ? ?
puts("Got TPM response:\n");
+ ? ? ? ? ? ? for (i = 0; i <read_size; i++)
+ ? ? ? ? ? ? ? ? ? ?printf(" %2.2x", tpm_buffer[i]);
+ ? ? ? ? ? ?puts("\n");
+ ? ? ? ? ? ? rv = 0;
+ ? ? } else {
+ ? ? ? ? ? ?puts("tpm command failed\n");
+ ? ? }
+ ? ? return rv;
+}
Quoted 2 lines
You might want to use
debug()where fitting ?
I don't expect failures and if happen prefer them to be printed always, not only if debug is enabled.
Quoted 6 lines
Quoted 6 lines
common/cmd_tpm.c new file +2
-0+ +static int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * constargv[]) +{
+ ? ? int rv = 0;
+
+ ? ? /*
Quoted 9 lines
Quoted 6 lines
+ ? ? ?* Verify that in case it is present, the first argument, it is
+ ? ? ?* exactly one character in size.
+ ? ? ?*/
+ ? ? if (argc < 7) {
+ ? ? ? ? ? ?puts("command should be at least six bytes in size\n");
+ ? ? ? ? ? ? return ~0;Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
I was under impression that any nonzero value is good. I see sometimes -1 returned for error in other u-boot sources. Also, I am sorry, I am new to this, when someone says "it is weird" - does this mean that it has to be changed?
Quoted 12 lines
Quoted 12 lines
+ ? ? }
common/cmd_tpm.c new file +11
-0+ + ? ? if (tis_init()) { + ? ? ? ? ? ? puts("tis_init() failed!\n"); + ? ? ? ? ? ? return ~0; + ? ? } + + ? ? if (tis_open()) { + ? ? ? ? ? ? puts("tis_open() failed!\n"); + ? ? ? ? ? ? return ~0; + ? ? } +
Quoted 9 lines
Quoted 5 lines
+ ? ? rv =
tpm_process(argc - 1, argv + 1);
+
+ ? ? if (!rv &&tis_close()) {
+ ? ? ? ? ? ?puts("tis_close() failed!\n");
+ ? ? ? ? ? ? rv = ~0;This doesn't make much sense,
tis_close()might not be called under all circumstances, is it ok ?
good point, I thought about this, but left untouched. It does not matter with this driver, but is better to call tis_close() no matter what. I'll fix it.
Quoted 7 lines
Quoted 7 lines
+ ? ? }
common/cmd_tpm.c new file +6
-0+ + ? ? return rv; +} + +U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm, + ? ? ? ?"tpm <data> [<data>] ? - write data and read ressponse\n",
Quoted 8 lines
common/cmd_tpm.c
Quoted 6 lines
+ ? ? ? ?"send arbitrary data to the TPM and read the response"
common/cmd_tpm.c new file +5
-0+); + +static void report_error (const char *msg) +{ + ? ? if (msg && *msg)Uhm, you also check if first character is non-zero? why ?
To avoid printing an empty string if someone calls this with an empty message?
Quoted 6 lines
Quoted 3 lines
+ ? ? ? ? ? ?
printf("%s\n", msg);
+ ? ?cmd_usage(&__u_boot_cmd_tpm);Two underscores aren't a good practice.
I did this as a result of a previous review. Do you have a suggestion how this should be done instead?
cheers,
/vb
Quoted 4 lines
Quoted 1 line
+}
Cheers
PATCH v2 2/2 Add a cli command to test the TPM device.
Dear Vadim Bendebury,
Quoted 9 lines · In message <CAC3GErHaAGX39XjD04MnJWe3sa9XC087LLpf6SycVC6K7SLt6Q@mail.gmail.com>…
Quoted 6 lines
Quoted 3 lines
+ * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+ * Released under the 2-clause BSD license.Are we ok with this ? Also, you say something about GPL in the same comment?
Can someone please tell me what needs to be put in the license headers and I will do it. I hear different suggestions from different people.
See previous comment - drop the BSD part if you include a GPLv2+ license header.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ return ~0;
Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
I was under impression that any nonzero value is good. I see sometimes -1 returned for error in other u-boot sources. Also, I am sorry, I am new to this, when someone says "it is weird" - does this mean that it has to be changed?
Commands are running in some sort of shell environment. SO please return 0 for OK and 1 for general errors like all other commands do (or should do).
...
Quoted 7 lines
Quoted 5 lines
common/cmd_tpm.c line 108
Quoted 3 lines
common/cmd_tpm.c new file +3
-0+static void report_error (const char *msg) +{ +if (msg && *msg)Uhm, you also check if first character is non-zero? why ?
To avoid printing an empty string if someone calls this with an empty message?
It's your code, so just don't do it, then.
And what's wrong about printing an empty string? YOuy're just adding dead code (and increased memory footprint) here.
Quoted 4 lines
Quoted 1 line
Two underscores aren't a good practice.
I did this as a result of a previous review. Do you have a suggestion how this should be done instead?
First, and most important, __u_boot_cmd_tpm appears to be undefined in your two patches, so it looks to be a real bug.
Second, please read the C standard's section about reserved identifiers.
Best regards,
Wolfgang Denk
Signature · 8 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The universe contains any amount of horrible ways to be woken up,
such as the noise of the mob breaking down the front door, the scream
of fire engines, or the realization that today is the Monday which on
Friday night was a comfortably long way off.
- Terry Pratchett, _Moving Pictures_PATCH v2 2/2 Add a cli command to test the TPM device.
Quoted 11 lines · On Saturday 15 October 2011 15:44:04 Wolfgang Denk wrote:
Quoted 4 lines · Vadim Bendebury
Quoted 1 line
Two underscores aren't a good practice.
I did this as a result of a previous review. Do you have a suggestion how this should be done instead?
First, and most important,
__u_boot_cmd_tpmappears to be undefined in your two patches, so it looks to be a real bug.Second, please read the C standard's section about reserved identifiers.
no, this is coming from common u-boot code. look at include/command.h:U_BOOT_CMD defines.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111015/4d36d91f/attachment.pgp
PATCH v2 2/2 Add a cli command to test the TPM device.
Quoted 10 lines · Mike Frysinger
Quoted 9 lines · On Saturday 15 October 2011 15:44:04 Wolfgang Denk wrote:
Quoted 4 lines · Vadim Bendebury
Quoted 1 line
Two underscores aren't a good practice.
I did this as a result of a previous review. Do you have a suggestion how this should be done instead?
First, and most important,
__u_boot_cmd_tpmappears to be undefined in your two patches, so it looks to be a real bug.
Quoted 7 lines
Quoted 2 lines
Second, please read the C standard's section about reserved identifiers.
no, this is coming from common u-boot code. ?look at include/command.h:U_BOOT_CMD defines.
-mike
or, more importantly: the question is what is the right/suggested way to print out the help message associated with a U_BOOT_CMD declaration?
cheers,
/vb
PATCH v2 2/2 Add a cli command to test the TPM device.
Quoted 14 lines · On Saturday 15 October 2011 16:27:02 Vadim Bendebury wrote:
Quoted 13 lines · Mike Frysinger
Quoted 11 lines · On Saturday 15 October 2011 15:44:04 Wolfgang Denk wrote:
Quoted 4 lines · Vadim Bendebury
Quoted 1 line
Two underscores aren't a good practice.
I did this as a result of a previous review. Do you have a suggestion how this should be done instead?
First, and most important,
__u_boot_cmd_tpmappears to be undefined in your two patches, so it looks to be a real bug.Second, please read the C standard's section about reserved identifiers.
Quoted 6 lines
Quoted 2 lines
no, this is coming from common u-boot code. look at include/command.h:U_BOOT_CMD defines.
or, more importantly: the question is what is the right/suggested way to print out the help message associated with a
U_BOOT_CMDdeclaration?
your command is given a cmd_tbl_t *cmdtp pointer to pass to cmd_usage -mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111016/2ef767da/attachment.pgp
PATCH v2 2/2 Add a cli command to test the TPM device.
In reply to Wolfgang DenkDear Wolfgang Denk,
Quoted 7 lines · Wolfgang Denk
Dear Vadim Bendebury,
Quoted 4 lines · In message <CAC3GErHaAGX39XjD04MnJWe3sa9XC087LLpf6SycVC6K7SLt6Q@mail.gmail.com>…
Quoted 4 lines
Quoted 3 lines
+ * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+ * Released under the 2-clause BSD license.
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
Are we ok with this ? Also, you say something about GPL in the same comment?
Can someone please tell me what needs to be put in the license headers and I will do it. I hear different suggestions from different people.
See previous comment - drop the BSD part if you include a GPLv2+ license header.
done
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
+ ? ? ? ? ? ? return ~0;
Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
Quoted 9 lines
Quoted 4 lines
I was under impression that any nonzero value is good. I see sometimes -1 returned for error in other u-boot sources. Also, I am sorry, I am new to this, when someone says "it is weird" - does this mean that ?it has to be changed?
Commands are running in some sort of shell environment. ?SO please return 0 for OK and 1 for general errors like all other commands do (or should do).
done
Quoted 16 lines
...
Quoted 7 lines
Quoted 5 lines
common/cmd_tpm.c
Quoted 3 lines
common/cmd_tpm.c new file +3
-0+static void report_error (const char *msg) +{ + ? if (msg && *msg)Uhm, you also check if first character is non-zero? why ?
To avoid printing an empty string if someone calls this with an empty message?
It's your code, so just don't do it, then.
And what's wrong about printing an empty string? ?YOuy're just adding dead code (and increased memory footprint) here.
Quoted 2 lines
Quoted 1 line
Two underscores aren't a good practice.
Quoted 9 lines
Quoted 2 lines
I did this as a result of a previous review. Do you have a suggestion how this should be done instead?
First, and most important,
__u_boot_cmd_tpmappears to be undefined in your two patches, so it looks to be a real bug.Second, please read the C standard's section about reserved identifiers.
reworked to avoid all the complications.
cheers,
/vb
Quoted 14 lines
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de The universe contains any amount of horrible ways ?to ?be ?woken ?up, such as the noise of the mob breaking down the front door, the scream of fire engines, or the realization that today is the Monday which on Friday night was a comfortably long way off.
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? - Terry Pratchett,_MovingPictures_
PATCH v2 2/2 Add a cli command to test the TPM device.
In reply to Marek VasutQuoted 10 lines · On Saturday 15 October 2011 14:02:29 Marek Vasut wrote:
common/cmd_tpm.c line 3
Quoted 6 lines · Vadim Bendebury
common/cmd_tpm.c new file +3
-0@@ -0,0 +1,111 @@ +/* +* Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +* Released under the 2-clause BSD license. Are we ok with this ? Also, you say something about GPL in the same comment?
there's nothing wrong with adding files under the BSD license. what is odd about this code though is that it says BSD on one line, and then it says GPL-2+ a few lines later. pick one or the other.
Quoted 9 lines
common/cmd_tpm.c line 78
Quoted 7 lines
common/cmd_tpm.c new file +7
-0+/* +* Verify that in case it is present, the first argument, it is +* exactly one character in size. +*/ +if (argc <7 ) { +puts ("command should be at least six bytes in size\n" ); +return ~0 ;Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
~0 is weird. this should be 1 or -1.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111015/1452c232/attachment.pgp
PATCH v2 2/2 Add a cli command to test the TPM device.
Quoted 12 lines · Mike Frysinger
Quoted 10 lines · On Saturday 15 October 2011 14:02:29 Marek Vasut wrote:
common/cmd_tpm.c line 3
Quoted 6 lines · Vadim Bendebury
common/cmd_tpm.c new file +3
-0@@ -0,0 +1,111 @@ +/* +* Copyright (c) 2011 The Chromium OS Authors. All rights reserved. +* Released under the 2-clause BSD license. Are we ok with this ? Also, you say something about GPL in the same comment?
Quoted 4 lines
there's nothing wrong with adding files under the BSD license. ?what is odd about this code though is that it says BSD on one line, and then it says GPL-2+ a few lines later. ?pick one or the other.
done
Quoted 11 lines
Quoted 9 lines
common/cmd_tpm.c line 10
Quoted 7 lines
common/cmd_tpm.c new file +7
-0+ ? /* + ? ?* Verify that in case it is present, the first argument, it is + ? ?* exactly one character in size. + ? ?*/ + ? if (argc < 7) { + ? ? ? ? ? puts("command should be at least six bytes in size\n"); + ? ? ? ? ? return ~0;Ugh, return 1 isn't ok ? Using ~0 on int type is weird.
~0 is weird. ?this should be 1 or -1.
done
Quoted 2 lines
-mike
PATCH v2 2/2 Add a cli command to test the TPM device.
In reply to Vadim BendeburyQuoted 4 lines · On Friday 14 October 2011 23:39:08 Vadim Bendebury wrote:
common/cmd_tpm.c new file +1
-0+/*
common/cmd_tpm.c line 102
Quoted 10 lines
common/cmd_tpm.c new file +9
-0+* Verify that in case it is present, the first argument, it is +* exactly one character in size. +*/ +if (argc <7 ) { +puts ("command should be at least six bytes in size\n" ); +return ~0 ; + } +U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm, + "tpm <data> [<data>] - write data and read ressponse\n",
the usage information does not convey that you have to do:
tpm 1 2 3 4 5 6 7 8 9
perhaps says "<byte> [<byte> ...]" instead ? and note that you have to specify at least 6 ?
also, there's a typo: ressponse -> response
common/cmd_tpm.c line 111
Quoted 6 lines
common/cmd_tpm.c new file +6
-0+static void report_error (const char *msg) +{ +if (msg && *msg) +printf ("%s\n" , msg); +cmd_usage (&__u_boot_cmd_tpm); +}
this gets used in one place, and the one place where it does get used, i don't see a point in calling cmd_usage(). just have the one place where this is used call puts() instead.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111015/af49390d/attachment.pgp
PATCH v2 2/2 Add a cli command to test the TPM device.
In reply to Vadim Bendeburycommon/Makefile COBJS-$(CONFIG_CMD_UBI) += cmd_ubi.o · line 156
Quoted 7 lines · On Friday 14 October 2011 23:39:08 Vadim Bendebury wrote:
common/Makefile +1
-0COBJS-$(CONFIG_CMD_UBIFS) += cmd_ubifs.o COBJS-$(CONFIG_CMD_UNIVERSE) += cmd_universe.o COBJS-$(CONFIG_CMD_UNZIP) += cmd_unzip.o +COBJS-$(CONFIG_CMD_TPM) += cmd_tpm.o
keep the list sorted
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111015/1e19d948/attachment.pgp