[tpm2-tools] Uprev from v2.0.0 to v3.1.3#98
Conversation
|
Merge with OpenXT/xenclient-oe#1062 |
| generate_policy "${own_key}" | ||
|
|
||
| if [ $? -ne 0 ]; then | ||
| echo "error generating tboot policy" >&2 |
There was a problem hiding this comment.
Is there any reason to log the failure here? From what I can tell, everywhere in generate_policy when an error is encountered it messages specifically why it failed before returning the non-zero return value. IMHO, if there is a failure in generate_policy that is not does not have a corresponding log message, it should be fixed there instead of a course grain message at this level.
There was a problem hiding this comment.
I added this more for the return 1 rather than the log. This function (own_tpm) will return 0 whether or not generate_policy fails, and that was causing some headaches while debugging.
There was a problem hiding this comment.
Apologies, as I should have been more clear. I am good with the return of 1, I wasn't sure if the additional log message was because a failure was missed. If it was then that should be fixed in the generate_policy function. If it wasn't, then this log message is noise which is something we should be looking to reduce.
There was a problem hiding this comment.
There was no reason for the log itself, no. I will remove that and let generate_policy handle the logging, which seems to be pretty thorough.
tsirakisn
force-pushed
the
tpm2-uprev
branch
from
c48f1c0 to
adf06c7
Compare
| generate_policy "${own_key}" | ||
|
|
||
| return 0 | ||
| return $? |
There was a problem hiding this comment.
I think this will break OTA Upgrading. generate_policy was previously silently ignored but will now fail and fail the upgrade. An OTA doesn't have the TPM password available, so it cannot write TPM nvram.
The fix for that is probably to change commit_dom0() to only call own_tpm() when [ "${MEASURE_LAUNCH}" = "true" ] && [ "${INSTALL_MODE}" = "fresh" ]
There was a problem hiding this comment.
Also, return $? is redundant in this context.
There was a problem hiding this comment.
@jandryuk good point, I don't see any reason to reclaim tpm ownership on an upgrade. In this case I think it would be best to keep the return $?, would you agree? @eric-ch it's possible for generate_policy to return a non-zero value, in which case I would think the user should be alerted and the installation should stop.
There was a problem hiding this comment.
@tsirakisn return $? is redundant since the exit status of the last statement of a function becomes the exit status for the function. On the other hand, I feel that return $? makes it explicit that we want to passing along the exit status. I am undecided, so we can just follow @eric-ch 's suggestion.
@tsirakisn when you respin this, it should be two patches since they are two distinct changes. The first should re-work generate_policy and own_tpm and explain why it is doing so. The second should change the tpm command for uprev.
The TPM should already be owned properly if we are upgrading. This enables the installer to fail out correctly when we can't generate the policy, rather than silently ignoring it for the sake of supporting the upgrade case. Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
tsirakisn
force-pushed
the
tpm2-uprev
branch
from
adf06c7 to
316ee82
Compare
|
Merging soon. |
5 participants
Supporting installer changes for #1062
OXT-1456
Signed-off-by: Nicholas Tsirakis tsirakisn@ainfosec.com