Exhaustive test for rollback code

This test sets the TPM to a each of a large amount of "interesting" initial states, and runs the firmware code at user level.

This code compiles and installs using a modified ebuild (which needs to be committed after this change).

[Luigi Semenzato, 2010-07-01 22:11:32]

This is the initial version of the rollback test.  It probably still has bugs,
but I've already exposed a few rollback code bugs, and I'd like to fix those
before I continue with this, and there are some dependencies so it's easier to
commit this now.  Also, it's useful to get feedback at this point.

[gauravsh, 2010-07-01 23:10:05]

your patch seems to have picked up changes from your previous CLs. Did you run
git remote update and merge on the CL branch, and re-upload?

[gauravsh, 2010-07-02 01:52:50]

i know this is work in progress - but here is some initial bit of feedback.

http://codereview.chromium.org/2857030/diff/6001/7010
File tests/rollback_index_test.c (right):

http://codereview.chromium.org/2857030/diff/6001/7010#newcode31
tests/rollback_index_test.c:31: #define RETURN_ON_FAILURE(tpm_command) do {     
       \
this macro is repeated in many of the tests, can you refactor this into a
separate file with common functionality for all the tpm_lite tests?

http://codereview.chromium.org/2857030/diff/6001/7010#newcode49
tests/rollback_index_test.c:49: * There are a total of N SafeWrite() call sites,
where N = 12.  Every call
I know this is very much WIP, but this comment needs some elaboration. If it
gets too crowded in the source file, you may also consider moving the
description of what this test does and how it works to a separate README.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode70
tests/rollback_index_test.c:70: int advancing;
comments for the state variable?

http://codereview.chromium.org/2857030/diff/6001/7010#newcode104
tests/rollback_index_test.c:104: status = system("/sbin/reboot");
do we want the reboot to be automatic, or would it be better to prompt the user
to reboot and run the test again?

http://codereview.chromium.org/2857030/diff/6001/7010#newcode182
tests/rollback_index_test.c:182: status = system("tcsd && sleep 0.1 &&
tpm_takeownership -y -z && "
i would use absolute paths for external commands - in case the shell path
variable is all messed up.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode225
tests/rollback_index_test.c:225: static uint32_t RollbackTest_SetTPMState(int
initialize) {
can this be split into smaller functions. it's too large a hunk of code.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode390
tests/rollback_index_test.c:390: RBTS.rbts_field = from;              \
this is a bit confusing. should from and to be reversed here? If not, what is
the macro trying to do?

http://codereview.chromium.org/2857030/diff/6001/7010#newcode400
tests/rollback_index_test.c:400: /* RBT.phase indicates how to count.
i don't understand this comment

http://codereview.chromium.org/2857030/diff/6001/7010#newcode433
tests/rollback_index_test.c:433: static void RollbackTest_InitializeState(void)
{
so I don't see the initialization of the state variable RBTS. Is the assumption
that since it is a global, it will be all zero-ed out?

http://codereview.chromium.org/2857030/diff/6001/7010#newcode491
tests/rollback_index_test.c:491: FILE* file = fopen(STATEPATH, "r+");
will this create a new file if one doesn't already exist?

http://codereview.chromium.org/2857030/diff/6001/7010#newcode496
tests/rollback_index_test.c:496: if (getuid() != 0) {
geteuid() is a more accurate call for this.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode501
tests/rollback_index_test.c:501: arg = argv[1];
why is this necessary. since there is only 1 (optional argument), you can just
use argv[] in the comparison below.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode508
tests/rollback_index_test.c:508: RollbackTest_InitializeState();
ok, ignore my previous comment about fopen() - you explicitly create a new file
with initialize command line option.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode513
tests/rollback_index_test.c:513: } else if (argc == 1) {
this if is unnecessary. argc is always >=1 and you already check for the >1
condition above.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode551
tests/rollback_index_test.c:551: fprintf(stderr, "usage: rollback-test [
initialize ]\n");
move this usage check condition to the beginning. nested if's and the many
if-blocks make the code slightly hard to parse at a glance.

[Luigi Semenzato, 2010-07-02 22:53:32]

Still WIP.  This already fixes some problems with the rollback_index code that
were revealed while testing.

I have progressed to about 9 iterations of the test.

I have addressed the issues Gaurav raised, except for the ones for which I added
questions/clarifications, and the large, ugly TPM state setting function (not
done yet).

Thanks!

http://codereview.chromium.org/2857030/diff/6001/7010
File tests/rollback_index_test.c (right):

http://codereview.chromium.org/2857030/diff/6001/7010#newcode31
tests/rollback_index_test.c:31: #define RETURN_ON_FAILURE(tpm_command) do {     
       \
On 2010/07/02 01:52:50, gauravsh wrote:
> this macro is repeated in many of the tests, can you refactor this into a
> separate file with common functionality for all the tpm_lite tests?

It's only used in rollback_index.c and rollback_index_test.c.  I couldn't find a
good place for it.  I judged that the duplication was better than introducing a
new file.  Any suggestion?

http://codereview.chromium.org/2857030/diff/6001/7010#newcode104
tests/rollback_index_test.c:104: status = system("/sbin/reboot");
On 2010/07/02 01:52:50, gauravsh wrote:
> do we want the reboot to be automatic, or would it be better to prompt the
user
> to reboot and run the test again?

The test can be quite long.  The reboot could be done from a script, upon
certain exit values, but I thought this would be simpler.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode182
tests/rollback_index_test.c:182: status = system("tcsd && sleep 0.1 &&
tpm_takeownership -y -z && "
On 2010/07/02 01:52:50, gauravsh wrote:
> i would use absolute paths for external commands - in case the shell path
> variable is all messed up.

Yes---in production code that would be better.  But in this case, it's more
likely that the paths for those binaries will change, than the path will be set
incorrectly.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode433
tests/rollback_index_test.c:433: static void RollbackTest_InitializeState(void)
{
On 2010/07/02 01:52:50, gauravsh wrote:
> so I don't see the initialization of the state variable RBTS. Is the
assumption
> that since it is a global, it will be all zero-ed out?

Yes.  Do you prefer explicit initialization?

[Luigi Semenzato, 2010-07-08 00:09:08]

PTAL.  The test seems to be running fine on a couple of ST8s.  The test only
runs when the ethernet cable is connected.  This is a safety measure to make it
easier to stop the test, even though some runs are so slow (because of
tpm_takeownership) that in retrospect this feature wasn't needed :/

[gauravsh, 2010-07-08 02:24:44]

lgtm with comments about comments

http://codereview.chromium.org/2857030/diff/6001/7010
File tests/rollback_index_test.c (right):

http://codereview.chromium.org/2857030/diff/6001/7010#newcode31
tests/rollback_index_test.c:31: #define RETURN_ON_FAILURE(tpm_command) do {     
       \
On 2010/07/02 22:53:32, Luigi Semenzato wrote:
> On 2010/07/02 01:52:50, gauravsh wrote:
> > this macro is repeated in many of the tests, can you refactor this into a
> > separate file with common functionality for all the tpm_lite tests?
> 
> It's only used in rollback_index.c and rollback_index_test.c.  I couldn't find
a
> good place for it.  I judged that the duplication was better than introducing
a
> new file.  Any suggestion?
> 
>

Ok, maybe I was just imaging the repetition then. Fine as is.

http://codereview.chromium.org/2857030/diff/6001/7010#newcode182
tests/rollback_index_test.c:182: status = system("tcsd && sleep 0.1 &&
tpm_takeownership -y -z && "
On 2010/07/02 22:53:32, Luigi Semenzato wrote:
> On 2010/07/02 01:52:50, gauravsh wrote:
> > i would use absolute paths for external commands - in case the shell path
> > variable is all messed up.
> 
> Yes---in production code that would be better.  But in this case, it's more
> likely that the paths for those binaries will change, than the path will be
set
> incorrectly.

That's a valid point. Feel free to switch to non-absolute paths.

> 
>

http://codereview.chromium.org/2857030/diff/30001/31010
File tests/rbtest.conf (right):

http://codereview.chromium.org/2857030/diff/30001/31010#newcode15
tests/rbtest.conf:15: # Connecting to tcsd requires networking.
this comment seems unclear to me. are you suggesting that the device must be
connected to the network or just that tcsd uses local sockets for communication?

http://codereview.chromium.org/2857030/diff/30001/31011
File tests/rollback_index_test.c (right):

http://codereview.chromium.org/2857030/diff/30001/31011#newcode55
tests/rollback_index_test.c:55: const int low_writecount = 56;
what do these constants represent?

[Luigi Semenzato, 2010-07-08 15:16:51]

Thank you for the review.  I will add some clarifications and push.

http://codereview.chromium.org/2857030/diff/6001/7010
File tests/rollback_index_test.c (right):

http://codereview.chromium.org/2857030/diff/6001/7010#newcode182
tests/rollback_index_test.c:182: status = system("tcsd && sleep 0.1 &&
tpm_takeownership -y -z && "
On 2010/07/08 02:24:45, gauravsh wrote:
> On 2010/07/02 22:53:32, Luigi Semenzato wrote:
> > On 2010/07/02 01:52:50, gauravsh wrote:
> > > i would use absolute paths for external commands - in case the shell path
> > > variable is all messed up.
> > 
> > Yes---in production code that would be better.  But in this case, it's more
> > likely that the paths for those binaries will change, than the path will be
> set
> > incorrectly.
> 
> That's a valid point. Feel free to switch to non-absolute paths.
> 
> > 
> > 
> 
> 

:-) I just switched to absolute paths.  Not a big deal either way.

http://codereview.chromium.org/2857030/diff/30001/31010
File tests/rbtest.conf (right):

http://codereview.chromium.org/2857030/diff/30001/31010#newcode15
tests/rbtest.conf:15: # Connecting to tcsd requires networking.
On 2010/07/08 02:24:45, gauravsh wrote:
> this comment seems unclear to me. are you suggesting that the device must be
> connected to the network or just that tcsd uses local sockets for
communication?

I am going to expand on this comment.  The requirement is that "localhost" be
available for connections.  Apparently it's not enough that flimflam be started,
I still get a lot of connection failures.  So I put the extra "ping localhost"
in the while loop below, and now the test works all the times.

http://codereview.chromium.org/2857030/diff/30001/31011
File tests/rollback_index_test.c (right):

http://codereview.chromium.org/2857030/diff/30001/31011#newcode55
tests/rollback_index_test.c:55: const int low_writecount = 56;
On 2010/07/08 02:24:45, gauravsh wrote:
> what do these constants represent? 

I will add comments, thanks.

[gauravsh, 2010-07-08 16:40:17]

On Thu, Jul 8, 2010 at 8:16 AM,  <semenzato@chromium.org> wrote:
> Thank you for the review.  I will add some clarifications and push.
>
>
> http://codereview.chromium.org/2857030/diff/6001/7010
> File tests/rollback_index_test.c (right):
>
> http://codereview.chromium.org/2857030/diff/6001/7010#newcode182
> tests/rollback_index_test.c:182: status = system("tcsd && sleep 0.1 &&
> tpm_takeownership -y -z && "
> On 2010/07/08 02:24:45, gauravsh wrote:
>>
>> On 2010/07/02 22:53:32, Luigi Semenzato wrote:
>> > On 2010/07/02 01:52:50, gauravsh wrote:
>> > > i would use absolute paths for external commands - in case the
>
> shell path
>>
>> > > variable is all messed up.
>> >
>> > Yes---in production code that would be better.  But in this case,
>
> it's more
>>
>> > likely that the paths for those binaries will change, than the path
>
> will be
>>
>> set
>> > incorrectly.
>
>> That's a valid point. Feel free to switch to non-absolute paths.
>
>> >
>> >
>
>
>
> :-) I just switched to absolute paths.  Not a big deal either way.
>
> http://codereview.chromium.org/2857030/diff/30001/31010
> File tests/rbtest.conf (right):
>
> http://codereview.chromium.org/2857030/diff/30001/31010#newcode15
> tests/rbtest.conf:15: # Connecting to tcsd requires networking.
> On 2010/07/08 02:24:45, gauravsh wrote:
>>
>> this comment seems unclear to me. are you suggesting that the device
>
> must be
>>
>> connected to the network or just that tcsd uses local sockets for
>
> communication?
>
> I am going to expand on this comment.  The requirement is that
> "localhost" be available for connections.  Apparently it's not enough
> that flimflam be started, I still get a lot of connection failures.  So
> I put the extra "ping localhost" in the while loop below, and now the
> test works all the times.

I am not sure I understand this. Even if tcsd uses local sockets to
communicate - I don't see why it should have to wait for flimflam to
startup. The socket IPC system should already be up. Maybe it is
related to name resolution - Have you tried just using 127.0.0.1. Any
loopback address should work.

>
> http://codereview.chromium.org/2857030/diff/30001/31011
> File tests/rollback_index_test.c (right):
>
> http://codereview.chromium.org/2857030/diff/30001/31011#newcode55
> tests/rollback_index_test.c:55: const int low_writecount = 56;
> On 2010/07/08 02:24:45, gauravsh wrote:
>>
>> what do these constants represent?
>
> I will add comments, thanks.
>
> http://codereview.chromium.org/2857030/show
>



-- 
-g

[Luigi Semenzato, 2010-07-08 16:48:12]

Name resolution is a likely cause.  Unfortunately there is no
configuration option for trousers to use a different host (or a unix
domain socket), only a different port number.  I would prefer using
this little hack to changing the trousers source---it's just a testing
program.  I have spoken with Chris and Robert who also had a problem
with early access to tcsd, in fact it was Robert who suggested the
ping solution.


On Thu, Jul 8, 2010 at 9:40 AM, Gaurav Shah <gauravsh@chromium.org> wrote:
> On Thu, Jul 8, 2010 at 8:16 AM,  <semenzato@chromium.org> wrote:
>> Thank you for the review.  I will add some clarifications and push.
>>
>>
>> http://codereview.chromium.org/2857030/diff/6001/7010
>> File tests/rollback_index_test.c (right):
>>
>> http://codereview.chromium.org/2857030/diff/6001/7010#newcode182
>> tests/rollback_index_test.c:182: status = system("tcsd && sleep 0.1 &&
>> tpm_takeownership -y -z && "
>> On 2010/07/08 02:24:45, gauravsh wrote:
>>>
>>> On 2010/07/02 22:53:32, Luigi Semenzato wrote:
>>> > On 2010/07/02 01:52:50, gauravsh wrote:
>>> > > i would use absolute paths for external commands - in case the
>>
>> shell path
>>>
>>> > > variable is all messed up.
>>> >
>>> > Yes---in production code that would be better.  But in this case,
>>
>> it's more
>>>
>>> > likely that the paths for those binaries will change, than the path
>>
>> will be
>>>
>>> set
>>> > incorrectly.
>>
>>> That's a valid point. Feel free to switch to non-absolute paths.
>>
>>> >
>>> >
>>
>>
>>
>> :-) I just switched to absolute paths.  Not a big deal either way.
>>
>> http://codereview.chromium.org/2857030/diff/30001/31010
>> File tests/rbtest.conf (right):
>>
>> http://codereview.chromium.org/2857030/diff/30001/31010#newcode15
>> tests/rbtest.conf:15: # Connecting to tcsd requires networking.
>> On 2010/07/08 02:24:45, gauravsh wrote:
>>>
>>> this comment seems unclear to me. are you suggesting that the device
>>
>> must be
>>>
>>> connected to the network or just that tcsd uses local sockets for
>>
>> communication?
>>
>> I am going to expand on this comment.  The requirement is that
>> "localhost" be available for connections.  Apparently it's not enough
>> that flimflam be started, I still get a lot of connection failures.  So
>> I put the extra "ping localhost" in the while loop below, and now the
>> test works all the times.
>
> I am not sure I understand this. Even if tcsd uses local sockets to
> communicate - I don't see why it should have to wait for flimflam to
> startup. The socket IPC system should already be up. Maybe it is
> related to name resolution - Have you tried just using 127.0.0.1. Any
> loopback address should work.
>
>>
>> http://codereview.chromium.org/2857030/diff/30001/31011
>> File tests/rollback_index_test.c (right):
>>
>> http://codereview.chromium.org/2857030/diff/30001/31011#newcode55
>> tests/rollback_index_test.c:55: const int low_writecount = 56;
>> On 2010/07/08 02:24:45, gauravsh wrote:
>>>
>>> what do these constants represent?
>>
>> I will add comments, thanks.
>>
>> http://codereview.chromium.org/2857030/show
>>
>
>
>
> --
> -g
>

[Nathan Williams, 2010-07-08 17:42:46]

http://codereview.chromium.org/2857030/diff/30001/31010
File tests/rbtest.conf (right):

http://codereview.chromium.org/2857030/diff/30001/31010#newcode15
tests/rbtest.conf:15: # Connecting to tcsd requires networking.
On 2010/07/08 15:16:51, Luigi Semenzato wrote:
> On 2010/07/08 02:24:45, gauravsh wrote:
> > this comment seems unclear to me. are you suggesting that the device must be
> > connected to the network or just that tcsd uses local sockets for
> communication?
> 
> I am going to expand on this comment.  The requirement is that "localhost" be
> available for connections.  Apparently it's not enough that flimflam be
started,
> I still get a lot of connection failures.  So I put the extra "ping localhost"
> in the while loop below, and now the test works all the times.

This is puzzling. We put in a change that made localhost resolve to 127.0.0.1
even when there's no networking and no DNS - in /etc/nsswitch.conf we prefer
files to dns, and /etc/hosts has 127.0.0.1 for localhost. Are you seeing
something different, and localhost not resolving? If so, please comment on
http://crosbug.com/3483 .

[Luigi Semenzato, 2010-07-08 17:54:38]

On 2010/07/08 17:42:46, Nathan Williams wrote:
> http://codereview.chromium.org/2857030/diff/30001/31010
> File tests/rbtest.conf (right):
> 
> http://codereview.chromium.org/2857030/diff/30001/31010#newcode15
> tests/rbtest.conf:15: # Connecting to tcsd requires networking.
> On 2010/07/08 15:16:51, Luigi Semenzato wrote:
> > On 2010/07/08 02:24:45, gauravsh wrote:
> > > this comment seems unclear to me. are you suggesting that the device must
be
> > > connected to the network or just that tcsd uses local sockets for
> > communication?
> > 
> > I am going to expand on this comment.  The requirement is that "localhost"
be
> > available for connections.  Apparently it's not enough that flimflam be
> started,
> > I still get a lot of connection failures.  So I put the extra "ping
localhost"
> > in the while loop below, and now the test works all the times.
> 
> This is puzzling. We put in a change that made localhost resolve to 127.0.0.1
> even when there's no networking and no DNS - in /etc/nsswitch.conf we prefer
> files to dns, and /etc/hosts has 127.0.0.1 for localhost. Are you seeing
> something different, and localhost not resolving? If so, please comment on
> http://crosbug.com/3483 .

Oh, I know.  I am running the tests on a fairly old version of Chrome OS,
probably from before you fixed the localhost issue.

Thanks!

[Luigi Semenzato, 2010-07-08 18:03:29]

It's unclear this change will ever be useful, but it is certainly more paranoid
than before, which is a good thing, I think.

[Luigi Semenzato, 2010-07-08 18:03:57]

On 2010/07/08 18:03:29, Luigi Semenzato wrote:
> It's unclear this change will ever be useful, but it is certainly more
paranoid
> than before, which is a good thing, I think.

Oops, sorry, I commented on the wrong CL.