diff mbox series

[v2,1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

Message ID 20201012234416.20995-2-kgoldman@us.ibm.com (mailing list archive)
State New, archived
Headers show
Series Updates to use IBM TSS C API rather than command line tools | expand

Commit Message

Ken Goldman Oct. 12, 2020, 11:44 p.m. UTC
The default value raw is appropriate for 'swtpm'.  tpm_server
uses the Microsoft packet encapsulation, so the env variable
must have the value mssim.

Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
---
 tests/boot_aggregate.test | 3 +++
 1 file changed, 3 insertions(+)

Comments

Mimi Zohar Oct. 14, 2020, 10:04 p.m. UTC | #1
Hi Ken,

On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
> The default value raw is appropriate for 'swtpm'.  tpm_server
> uses the Microsoft packet encapsulation, so the env variable
> must have the value mssim.
> 
> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>

Thank you for noticing this regression.

> ---
>  tests/boot_aggregate.test | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
> index 1c7b1f2..b109a32 100755
> --- a/tests/boot_aggregate.test
> +++ b/tests/boot_aggregate.test
> @@ -35,6 +35,7 @@ else
>  	export TPM_COMMAND_PORT=2321
>  	export TPM_PLATFORM_PORT=2322
>  	export TPM_SERVER_NAME="localhost"
> +	# swtpm uses the raw, unencapsulated packet format
>  	export TPM_SERVER_TYPE="raw"

Instead of adding a comment here, how about only exporting
TPM_SERVER_TYPE for "swtpm".

>  
>  fi
> @@ -73,6 +74,8 @@ swtpm_start() {
>  			SWTPM_PPID=$!
>  		fi
>  	elif [ -n "${swtpm}" ]; then
> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
> +                export TPM_SERVER_TYPE="mssim"

Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
fail.

thanks,

Mimi

>  		pgrep swtpm
>  		if [ $? -eq 0 ]; then
>  			echo "INFO: Software TPM (tpm_server) already running"
Ken Goldman Oct. 14, 2020, 10:17 p.m. UTC | #2
On 10/14/2020 6:04 PM, Mimi Zohar wrote:
> Hi Ken,
> 
> On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
>> The default value raw is appropriate for 'swtpm'.  tpm_server
>> uses the Microsoft packet encapsulation, so the env variable
>> must have the value mssim.
>>
>> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
> 
> Thank you for noticing this regression.
> 
>> ---
>>   tests/boot_aggregate.test | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
>> index 1c7b1f2..b109a32 100755
>> --- a/tests/boot_aggregate.test
>> +++ b/tests/boot_aggregate.test
>> @@ -35,6 +35,7 @@ else
>>   	export TPM_COMMAND_PORT=2321
>>   	export TPM_PLATFORM_PORT=2322
>>   	export TPM_SERVER_NAME="localhost"
>> +	# swtpm uses the raw, unencapsulated packet format
>>   	export TPM_SERVER_TYPE="raw"
> 
> Instead of adding a comment here, how about only exporting
> TPM_SERVER_TYPE for "swtpm".

That certainly works.  I thought the idea was, "Make the
smallest change that fixes the problem."   Moving that
line under swtpm is a reasonable alternative.

I'd leave the comment.  I suspect many people
don't know about the Microsoft TPM packet format,
so the line would otherwise be confusing.

> 
>>   
>>   fi
>> @@ -73,6 +74,8 @@ swtpm_start() {
>>   			SWTPM_PPID=$!
>>   		fi
>>   	elif [ -n "${swtpm}" ]; then
>> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
>> +                export TPM_SERVER_TYPE="mssim"
> 
> Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
> fail.
> 

That's odd.  Are you saying that openssl uses the env variable
TPM_SERVER_TYPE?  What in openssl fails?  What's the error
message.
Mimi Zohar Oct. 14, 2020, 10:28 p.m. UTC | #3
On Wed, 2020-10-14 at 18:17 -0400, Ken Goldman wrote:
> On 10/14/2020 6:04 PM, Mimi Zohar wrote:
> > Hi Ken,
> > 
> > On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
> >> The default value raw is appropriate for 'swtpm'.  tpm_server
> >> uses the Microsoft packet encapsulation, so the env variable
> >> must have the value mssim.
> >>
> >> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
> > 
> > Thank you for noticing this regression.
> > 
> >> ---
> >>   tests/boot_aggregate.test | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
> >> index 1c7b1f2..b109a32 100755
> >> --- a/tests/boot_aggregate.test
> >> +++ b/tests/boot_aggregate.test
> >> @@ -35,6 +35,7 @@ else
> >>   	export TPM_COMMAND_PORT=2321
> >>   	export TPM_PLATFORM_PORT=2322
> >>   	export TPM_SERVER_NAME="localhost"
> >> +	# swtpm uses the raw, unencapsulated packet format
> >>   	export TPM_SERVER_TYPE="raw"
> > 
> > Instead of adding a comment here, how about only exporting
> > TPM_SERVER_TYPE for "swtpm".
> 
> That certainly works.  I thought the idea was, "Make the
> smallest change that fixes the problem."   Moving that
> line under swtpm is a reasonable alternative.

In this case, moving the line and adding the comment is the smallest
change.  To indicate this is a bug fix, you would add "Fixes:
f831508297cd ("Install the swtpm package, if available") in addition to
your Signed-off-by tag.

> 
> I'd leave the comment.  I suspect many people
> don't know about the Microsoft TPM packet format,
> so the line would otherwise be confusing.
> 
> > 
> >>   
> >>   fi
> >> @@ -73,6 +74,8 @@ swtpm_start() {
> >>   			SWTPM_PPID=$!
> >>   		fi
> >>   	elif [ -n "${swtpm}" ]; then
> >> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
> >> +                export TPM_SERVER_TYPE="mssim"
> > 
> > Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
> > fail.
> > 
> 
> That's odd.  Are you saying that openssl uses the env variable
> TPM_SERVER_TYPE?  What in openssl fails?  What's the error
> message.

"make check" is showing:


TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
3: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
4: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
5: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
6: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
7: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
8: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
9: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
INFO: Calculating the boot_aggregate (PCRs 0 - 9) for multiple banks
Failed to read any TPM PCRs
errno: No such file or directory (2)
SKIP: evmctl ima_boot_aggregate: 

thanks,

Mimi
Ken Goldman Oct. 15, 2020, 12:54 p.m. UTC | #4
On 10/14/2020 6:28 PM, Mimi Zohar wrote:
>>> Instead of adding a comment here, how about only exporting
>>> TPM_SERVER_TYPE for "swtpm".

>> That certainly works.  I thought the idea was, "Make the
>> smallest change that fixes the problem."   Moving that
>> line under swtpm is a reasonable alternative.

> In this case, moving the line and adding the comment is the smallest
> change.  To indicate this is a bug fix, you would add "Fixes:
> f831508297cd ("Install the swtpm package, if available") in addition to
> your Signed-off-by tag.
> 

The current patch adds one line.  This proposal adds one line and
moves another line.  It also changes the swtpm flow, which must
be tested.

It's OK with me.  Let me know.

Where should I add that "Fixes ..." text?  What is the exact format?
Ken Goldman Oct. 15, 2020, 1:04 p.m. UTC | #5
On 10/14/2020 6:28 PM, Mimi Zohar wrote:
 >>>>
 >>>>    fi
 >>>> @@ -73,6 +74,8 @@ swtpm_start() {
 >>>>    			SWTPM_PPID=$!
 >>>>    		fi
 >>>>    	elif [ -n "${swtpm}" ]; then
 >>>> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
 >>>> +                export TPM_SERVER_TYPE="mssim"
 >>> Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
 >>> fail.
 >>>
 >> That's odd.  Are you saying that openssl uses the env variable
 >> TPM_SERVER_TYPE?  What in openssl fails?  What's the error
 >> message.
 > "make check" is showing:
 >
 >
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 3: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 4: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 5: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 6: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 7: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 8: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 9: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > INFO: Calculating the boot_aggregate (PCRs 0 - 9) for multiple banks
 > Failed to read any TPM PCRs
 > errno: No such file or directory (2)
 > SKIP: evmctl ima_boot_aggregate:

Are you sure that this failure is within openssl?  It doesn't look
that way to me.

Were there perhaps more of those errors?  I suspect that because
the messages are labeled 3-9, but PCR 0-9 are read.

I don't know the test code.  My guess is:

- If there were 10 errors, the startup command is missing,
causing each PCR read to fail.

- If there were 9 errors, startup is being sent before each PCR read,
but only one is permitted.
Mimi Zohar Oct. 15, 2020, 1:36 p.m. UTC | #6
On Thu, 2020-10-15 at 08:54 -0400, Ken Goldman wrote:
> On 10/14/2020 6:28 PM, Mimi Zohar wrote:
> >>> Instead of adding a comment here, how about only exporting
> >>> TPM_SERVER_TYPE for "swtpm".
> 
> >> That certainly works.  I thought the idea was, "Make the
> >> smallest change that fixes the problem."   Moving that
> >> line under swtpm is a reasonable alternative.
> 
> > In this case, moving the line and adding the comment is the smallest
> > change.  To indicate this is a bug fix, you would add "Fixes:
> > f831508297cd ("Install the swtpm package, if available") in addition to
> > your Signed-off-by tag.
> > 
> 
> The current patch adds one line.  This proposal adds one line and
> moves another line.  It also changes the swtpm flow, which must
> be tested.
> 
> It's OK with me.  Let me know.
> 
> Where should I add that "Fixes ..." text?  What is the exact format?

The "Fixes" tag belongs in the patch description above your Signed-off-
by tag.  The format is:  Fixes: < commit number> < commit ttitle>

e.g. Fixes: f831508297cd ("Install the swtpm package, if available")

As this is a bug fix, please update the Subject line and post this
change independently of the other changes.  I've already tested the
suggested change.  Once the updated patch is posted, it will hopefully
be tested by the distros as well.

thanks,

Mimi
diff mbox series

Patch

diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
index 1c7b1f2..b109a32 100755
--- a/tests/boot_aggregate.test
+++ b/tests/boot_aggregate.test
@@ -35,6 +35,7 @@  else
 	export TPM_COMMAND_PORT=2321
 	export TPM_PLATFORM_PORT=2322
 	export TPM_SERVER_NAME="localhost"
+	# swtpm uses the raw, unencapsulated packet format
 	export TPM_SERVER_TYPE="raw"
 
 fi
@@ -73,6 +74,8 @@  swtpm_start() {
 			SWTPM_PPID=$!
 		fi
 	elif [ -n "${swtpm}" ]; then
+	        # tpm_server uses the Microsoft simulator encapsulated packet format
+                export TPM_SERVER_TYPE="mssim"
 		pgrep swtpm
 		if [ $? -eq 0 ]; then
 			echo "INFO: Software TPM (tpm_server) already running"