On 01/29/10 10:28, Klaus Heinrich Kiwi wrote:
> I'm still trying to understand how all of this works so bare with me.
>
responses inline...
...
>> - 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.
I think so, but I did not write the original code (and I think it is pretty
confusing and should be better documented).
>
> 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?
Yes, perhaps it should return some sort of "NOT FOUND" error in that case,
I don't think it's causing a problem in it's current state but only because
the caller doesn't check the return status. It only appears to be
called by "auth_mgr_swap_out" which only looks for non-zero results and doesn't
seem to care about the actual value.
I think there are a lot of places in trousers where function return codes are
not
being checked and bubbled up through the stack, but I haven't had time to
look through it thoroughly.
>> /* 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.
The original formatting was silly :)
>
>>
>> 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.
I think that resetting the tpm_handle and tcs_ctx to NULL is important because
somewhere else, there may be code that is checking them or trying to use them.
>
> Identation is wrong though.
Looks good on my system, I think (as you said), thunderbird mangled my spacing.
Next time I will submit the patches as attachments instead.
>> 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).
The "cont" flag usually corresponds to the "fContinueAuthSession" flag, which
the TPM
sets to indicate that it is still using that session handle. If it is "true",
we
don't need to flush it or clear it out because it is still in use.
The old code was decrementing the open_auth_sessions counter and ignoring the
"cont"
flag which causes it to get out of sync with the TPM, and there was a definite
session
leaking problem. The result of which forces a hard reboot to reset the TPM.
Handling
session handles is a critical bit of code because it can lead to a really
trivial DOS
of the TPM and thus any software that relies on it.
>
> Is this an 'unreachable' state or am I missing something here?
>
It may be an unreachable state. I don't know if "release_auth_handle" would
be called
if the handle were swapped out.
-Wyllys
------------------------------------------------------------------------------
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
