| 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 |
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"
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.
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
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?
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.
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 --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"
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(+)