I'm still trying to understand how all of this works so bare with me.
Some are comments inlined below:
On Wed, 2010-01-27 at 16:14 -0500, Wyllys Ingersoll wrote:
> The following patch for the tcs_auth_mgr.c module to fix some issues with
> cleaning up auth sessions correctly.
>
> -Wyllys Ingersoll
>
> --- src/tcs/tcs_auth_mgr.c.old Mon Aug 3 12:19:13 2009
> +++ src/tcs/tcs_auth_mgr.c Thu Nov 12 13:26:02 2009
> @@ -28,7 +28,6 @@
>
> MUTEX_DECLARE_EXTERN(tcsp_lock);
>
> -
> /* Note: The after taking the auth_mgr_lock in any of the functions below,
> the
> * mem_cache_lock cannot be taken without risking a deadlock. So, the
> auth_mgr
> * functions must be "self-contained" wrt locking */
> @@ -80,7 +79,7 @@
> TSS_RESULT
> auth_mgr_save_ctx(TCS_CONTEXT_HANDLE hContext)
> {
> - TSS_RESULT result;
> + TSS_RESULT result = TSS_SUCCESS;
> UINT32 i;
>
> for (i = 0; i < auth_mgr.auth_mapper_size; i++) {
> @@ -87,7 +86,6 @@
> if (auth_mgr.auth_mapper[i].full == TRUE &&
> auth_mgr.auth_mapper[i].swap == NULL &&
> auth_mgr.auth_mapper[i].tcs_ctx != hContext) {
> -
> LogDebug("Calling TPM_SaveAuthContext for TCS CTX
> %x. Swapping out: TCS %x "
> "TPM %x", hContext,
> auth_mgr.auth_mapper[i].tcs_ctx,
> auth_mgr.auth_mapper[i].tpm_handle);
> @@ -98,12 +96,11 @@
> LogDebug("TPM_SaveAuthContext failed: 0x%x",
> result);
> return result;
> }
> -
> - /* XXX should there be a break here? */
> + break;
> }
> }
>
> - return TSS_SUCCESS;
> + return result;
> }
If I understood the above function correctly, it is trying to find the
first auth session in the auth_mapper table that:
* is a valid auth session (auth_mgr.auth_mapper[i].full)
* is not already swapped-out (auth_mgr.auth_mapper[i].swap == NULL)
* is not sharing the same TCS context as the caller
(auth_mgr.auth_mapper[i].tcs_ctx != hContext)
The last condition, I suppose, is to prevent us from swapping-out an
auth-session that is potentially still being used.
Given all the above, what happens if the we simply can't find an auth
session that can be swapped? We never call TPM_SaveAuthContext() but we
still return TSS_SUCCESS, thus making auth_mgr_swap_out() believe we
were successful in swapping out. Is this really OK? Shouldn't we
initialize result to something !TSS_SUCCESS?
>
> /* if there's a TCS context waiting to get auth, wake it up or swap it in */
> @@ -218,8 +215,8 @@
>
> /* Ok, probably dealing with a 1.1 TPM */
> if (result == TPM_E_BAD_ORDINAL)
> - result = internal_TerminateHandle(
> -
> auth_mgr.auth_mapper[i].tpm_handle);
> + result = internal_TerminateHandle(
> +
> auth_mgr.auth_mapper[i].tpm_handle);
Let's avoid simple formatting changes.
>
> if (result == TCPA_E_INVALID_AUTHHANDLE) {
> LogDebug("Tried to close an invalid
> auth handle: %x",
> @@ -228,10 +225,14 @@
> LogDebug("TPM_TerminateHandle
> returned %d", result);
> }
> }
> + /* clear the slot */
> auth_mgr.open_auth_sessions--;
> auth_mgr.auth_mapper[i].full = FALSE;
> + auth_mgr.auth_mapper[i].tpm_handle = 0;
> + auth_mgr.auth_mapper[i].tcs_ctx = 0;
I haven't checked if this is really important (given that the original
supposedly deinitializes this entry by setting full = FALSE), but it
shouldn't harm.
Identation is wrong though.
> LogDebug("released auth for TCS %x TPM %x",
> tcs_handle,
> - auth_mgr.auth_mapper[i].tpm_handle);
> + auth_mgr.auth_mapper[i].tpm_handle);
> +
formatting change?
> auth_mgr_swap_in();
> }
> }
> @@ -264,14 +265,22 @@
> auth_mgr.auth_mapper[i].tpm_handle == tpm_auth_handle &&
> auth_mgr.auth_mapper[i].tcs_ctx == tcs_handle) {
> if (!cont) {
> - /* Only termininate when not in use anymore */
> - result =
> TCSP_FlushSpecific_Common(auth_mgr.auth_mapper[i].tpm_handle,
> -
> TPM_RT_AUTH);
> + /*
> + * This function should not be necessary, but
> + * if the main operation resulted in an error,
> + * the TPM may still hold the auth handle
> + * and it must be freed. Most of the time
> + * this call will result in
> TPM_E_INVALID_AUTHHANDLE
> + * error which can be ignored.
> + */
> + result = TCSP_FlushSpecific_Common(
> + auth_mgr.auth_mapper[i].tpm_handle,
> + TPM_RT_AUTH);
>
> /* Ok, probably dealing with a 1.1 TPM */
> if (result == TPM_E_BAD_ORDINAL)
> - result = internal_TerminateHandle(
> -
> auth_mgr.auth_mapper[i].tpm_handle);
> + result = internal_TerminateHandle(
> +
> auth_mgr.auth_mapper[i].tpm_handle);
formatting change?
>
> if (result == TCPA_E_INVALID_AUTHHANDLE) {
> LogDebug("Tried to close an invalid
> auth handle: %x",
> @@ -279,12 +288,22 @@
> } else if (result != TCPA_SUCCESS) {
> LogDebug("TPM_TerminateHandle
> returned %d", result);
> }
> +
> + if (result == TPM_SUCCESS) {
> + LogDebug("released auth for TCS %x
> TPM %x",
> +
> auth_mgr.auth_mapper[i].tcs_ctx, tpm_auth_handle);
> + }
> + /*
> + * Mark it as released, the "cont" flag
> indicates
> + * that it is no longer needed.
> + */
> + auth_mgr.open_auth_sessions--;
> + auth_mgr.auth_mapper[i].full = FALSE;
> + auth_mgr.auth_mapper[i].tpm_handle = 0;
> + auth_mgr.auth_mapper[i].tcs_ctx = 0;
> + auth_mgr_swap_in();
> }
> - auth_mgr.open_auth_sessions--;
> - auth_mgr.auth_mapper[i].full = FALSE;
> - LogDebug("released auth for TCS %x TPM %x",
> - auth_mgr.auth_mapper[i].tcs_ctx,
> tpm_auth_handle);
> - auth_mgr_swap_in();
> + /* If the cont flag is TRUE, we have to keep the
> handle */
I must admit I don't completely understand the meaning of calling
auth_mgr_release_auth_handle() with cont=TRUE, but I also noted that
there isn't code here to handle the case where this auth_session is
swapped out (i.e., deallocate blob, set swap=NULL etc).
Is this an 'unreachable' state or am I missing something here?
> }
> }
>
> @@ -563,4 +582,3 @@
>
> return result;
> }
> -
formatting change?
Thanks,
-Klaus
------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
TrouSerS-tech mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/trousers-tech
