| Message ID | 940e5cbca2cf08f7275d5e09d552a8500e18742c.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/8] tss: Fix handling of TPM_RH_NULL in intel-tss | expand |
On Sun, 2024-08-04 at 09:42 -0400, James Bottomley wrote: > The design of the intel-tss shim is to hide the difference between > the > internal and the external handles by doing the internal to external > transform on entry. Unfortunately, the NULL handle (TPM_RH_NULL, > 40000007) has two possible internal representations depending on > whether it's used to indicate no session or the null hierarcy. > > There is a bug in the intel-tss in that it uses the wrong internal > NULL handle to try to create the NULL seed primary (and thus fails). > Now that we're going to be using the NULL primary to salt sessions, > the Intel TSS shim needs fixing to cope with thi correctly. > > The fix is to do the correct transform to the internal hierarchy > representation on NULL hierarchy creation and to do the session > handle > conversion everywhere else. Additionally remove the intel_handle() > code which was supposed to do this: it's unused because 0 is never > passed in as a handle number. Going over all the internal to external handle conversions, I found one more use case that would produce a bug. This one isn't actually used in the openssl_tpm2_engine code, so it's an unmanifested bug but nevertheless it should be fixed to avoid problems later on. I'll fold the below fix into this patch. Regards, James --- diff --git a/src/include/intel-tss.h b/src/include/intel-tss.h index 3b8c18d..a2050ba 100644 --- a/src/include/intel-tss.h +++ b/src/include/intel-tss.h @@ -1271,6 +1271,19 @@ tpm2_handle_ext(TSS_CONTEXT *tssContext, TPM_HANDLE esysh) { TPM2_HANDLE realh = 0; + switch (esysh) { + case ESYS_TR_RH_OWNER: + return EXT_TPM_RH_OWNER; + case ESYS_TR_RH_PLATFORM: + return EXT_TPM_RH_PLATFORM; + case ESYS_TR_RH_ENDORSEMENT: + return EXT_TPM_RH_ENDORSEMENT; + case ESYS_TR_RH_NULL: + return EXT_TPM_RH_NULL; + case ESYS_TR_NONE: + return EXT_TPM_RH_NULL; + } + Esys_TR_GetTpmHandle(tssContext, esysh, &realh); return realh;
On Sun Aug 4, 2024 at 4:42 PM EEST, James Bottomley wrote: > The design of the intel-tss shim is to hide the difference between the > internal and the external handles by doing the internal to external > transform on entry. Unfortunately, the NULL handle (TPM_RH_NULL, > 40000007) has two possible internal representations depending on > whether it's used to indicate no session or the null hierarcy. > > There is a bug in the intel-tss in that it uses the wrong internal > NULL handle to try to create the NULL seed primary (and thus fails). > Now that we're going to be using the NULL primary to salt sessions, > the Intel TSS shim needs fixing to cope with thi correctly. > > The fix is to do the correct transform to the internal hierarchy > representation on NULL hierarchy creation and to do the session handle > conversion everywhere else. Additionally remove the intel_handle() > code which was supposed to do this: it's unused because 0 is never > passed in as a handle number. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > v2: reword commit message > > --- > src/include/intel-tss.h | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/src/include/intel-tss.h b/src/include/intel-tss.h > index 1870b4e..5b8db20 100644 > --- a/src/include/intel-tss.h > +++ b/src/include/intel-tss.h > @@ -251,14 +251,6 @@ intel_sess_helper(TSS_CONTEXT *tssContext, > TPM_HANDLE auth, TPMA_SESSION flags) > TPMA_SESSION_CONTINUESESSION | > flags); > } > > -static inline TPM_HANDLE > -intel_handle(TPM_HANDLE h) > -{ > - if (h == 0) > - return ESYS_TR_NONE; > - return h; > -} > - > static inline void > TSS_Delete(TSS_CONTEXT *tssContext) > { > @@ -937,8 +929,10 @@ tpm2_CreatePrimary(TSS_CONTEXT *tssContext, > TPM_HANDLE primaryHandle, > TPM2B_PUBLIC *opub; > TPM_RC rc; > > - /* FIXME will generate wrong value for NULL hierarchy */ > - primaryHandle = intel_handle(primaryHandle); > + > + /* TPM_RH_NULL is mapped to ESYS_TR_NONE, which won't work > here */ > + if (primaryHandle == TPM_RH_NULL) > + primaryHandle = INT_TPM_RH_NULL; > > outsideInfo.size = 0; > creationPcr.count = 0; > @@ -993,9 +987,7 @@ tpm2_StartAuthSession(TSS_CONTEXT *tssContext, > TPM_HANDLE tpmKey, > TPM_HANDLE *sessionHandle, > const char *bindPassword) > { > - bind = intel_handle(bind); > - tpmKey = intel_handle(tpmKey); > - if (bind != ESYS_TR_NONE) > + if (bind != TPM_RH_NULL) > intel_auth_helper(tssContext, bind, bindPassword); Not blaming the patch but just have hard time coping this. The most basic question is probably this: what is the application for INT_TPM_RH_NULL? Let's imagine that we have a flow chart describing Intel shim as a state machine. I decide to shoot it with these three stimulus: 1. INT_TPM_RH_NULL 2. TPM_RH_NULL 3. A valid handle What happens in each case to its state? > > return Esys_StartAuthSession(tssContext, tpmKey, bind, > ESYS_TR_NONE, BR, Jarkko
On Mon, 2024-08-05 at 00:28 +0300, Jarkko Sakkinen wrote: [...] > > --- a/src/include/intel-tss.h > > +++ b/src/include/intel-tss.h > > @@ -251,14 +251,6 @@ intel_sess_helper(TSS_CONTEXT *tssContext, > > TPM_HANDLE auth, TPMA_SESSION flags) > > TPMA_SESSION_CONTINUESESSION | > > flags); > > } > > > > -static inline TPM_HANDLE > > -intel_handle(TPM_HANDLE h) > > -{ > > - if (h == 0) > > - return ESYS_TR_NONE; > > - return h; > > -} > > - > > static inline void > > TSS_Delete(TSS_CONTEXT *tssContext) > > { > > @@ -937,8 +929,10 @@ tpm2_CreatePrimary(TSS_CONTEXT *tssContext, > > TPM_HANDLE primaryHandle, > > TPM2B_PUBLIC *opub; > > TPM_RC rc; > > > > - /* FIXME will generate wrong value for NULL hierarchy */ > > - primaryHandle = intel_handle(primaryHandle); > > + > > + /* TPM_RH_NULL is mapped to ESYS_TR_NONE, which won't work > > here */ > > + if (primaryHandle == TPM_RH_NULL) > > + primaryHandle = INT_TPM_RH_NULL; > > > > outsideInfo.size = 0; > > creationPcr.count = 0; > > @@ -993,9 +987,7 @@ tpm2_StartAuthSession(TSS_CONTEXT *tssContext, > > TPM_HANDLE tpmKey, > > TPM_HANDLE *sessionHandle, > > const char *bindPassword) > > { > > - bind = intel_handle(bind); > > - tpmKey = intel_handle(tpmKey); > > - if (bind != ESYS_TR_NONE) > > + if (bind != TPM_RH_NULL) > > intel_auth_helper(tssContext, bind, bindPassword); > > Not blaming the patch but just have hard time coping this. > > The most basic question is probably this: what is the application for > INT_TPM_RH_NULL? Ah, well, it turns out that the Intel TSS also isn't very performant and part of the performance loss is using a memory database for translating external TPM objects into internal ones. Some of the performance can be recovered by not doing this. It turns out that the engine code never uses volatile external handles, so the switch from volatile internal -> external and vice versa (which occurs on load followed by key operation) was eliminated and the code uses internal representation for volatile handles and external representation for all other handles. This scheme worked fine until non-volatile key indexes were introduced and then it broke down a bit. It would be logically very nice to redo the internal representation to be entirely contained within the intel tss shim, but then that would reintroduce the performance problem (although on the other hand, the Intel TSS is still twice as slow as the IBM TSS so perhaps this wouldn't be such a huge addition). Regards, James
On Mon Aug 5, 2024 at 5:48 AM EEST, James Bottomley wrote: > On Mon, 2024-08-05 at 00:28 +0300, Jarkko Sakkinen wrote: > [...] > > > --- a/src/include/intel-tss.h > > > +++ b/src/include/intel-tss.h > > > @@ -251,14 +251,6 @@ intel_sess_helper(TSS_CONTEXT *tssContext, > > > TPM_HANDLE auth, TPMA_SESSION flags) > > > TPMA_SESSION_CONTINUESESSION | > > > flags); > > > } > > > > > > -static inline TPM_HANDLE > > > -intel_handle(TPM_HANDLE h) > > > -{ > > > - if (h == 0) > > > - return ESYS_TR_NONE; > > > - return h; > > > -} > > > - > > > static inline void > > > TSS_Delete(TSS_CONTEXT *tssContext) > > > { > > > @@ -937,8 +929,10 @@ tpm2_CreatePrimary(TSS_CONTEXT *tssContext, > > > TPM_HANDLE primaryHandle, > > > TPM2B_PUBLIC *opub; > > > TPM_RC rc; > > > > > > - /* FIXME will generate wrong value for NULL hierarchy */ > > > - primaryHandle = intel_handle(primaryHandle); > > > + > > > + /* TPM_RH_NULL is mapped to ESYS_TR_NONE, which won't work > > > here */ > > > + if (primaryHandle == TPM_RH_NULL) > > > + primaryHandle = INT_TPM_RH_NULL; > > > > > > outsideInfo.size = 0; > > > creationPcr.count = 0; > > > @@ -993,9 +987,7 @@ tpm2_StartAuthSession(TSS_CONTEXT *tssContext, > > > TPM_HANDLE tpmKey, > > > TPM_HANDLE *sessionHandle, > > > const char *bindPassword) > > > { > > > - bind = intel_handle(bind); > > > - tpmKey = intel_handle(tpmKey); > > > - if (bind != ESYS_TR_NONE) > > > + if (bind != TPM_RH_NULL) > > > intel_auth_helper(tssContext, bind, bindPassword); > > > > Not blaming the patch but just have hard time coping this. > > > > The most basic question is probably this: what is the application for > > INT_TPM_RH_NULL? > > Ah, well, it turns out that the Intel TSS also isn't very performant > and part of the performance loss is using a memory database for > translating external TPM objects into internal ones. Some of the > performance can be recovered by not doing this. So INT_RH_NULL is a flag that translates to "do not translate to internal object if it is derived from the nulll seed?". I.e. is it some kind of skip flag? External presentation is any TPM object I guess, but what is conceptually the internal representation we are talking about here? I wonder why nobody ever got idea that all kinds of TPM daemons could use u64 for handles, and have future-proof robustness as in TPM2 handles are u32. BR, Jarkko
diff --git a/src/include/intel-tss.h b/src/include/intel-tss.h index 1870b4e..5b8db20 100644 --- a/src/include/intel-tss.h +++ b/src/include/intel-tss.h @@ -251,14 +251,6 @@ intel_sess_helper(TSS_CONTEXT *tssContext, TPM_HANDLE auth, TPMA_SESSION flags) TPMA_SESSION_CONTINUESESSION | flags); } -static inline TPM_HANDLE -intel_handle(TPM_HANDLE h) -{ - if (h == 0) - return ESYS_TR_NONE; - return h; -} - static inline void
The design of the intel-tss shim is to hide the difference between the internal and the external handles by doing the internal to external transform on entry. Unfortunately, the NULL handle (TPM_RH_NULL, 40000007) has two possible internal representations depending on whether it's used to indicate no session or the null hierarcy. There is a bug in the intel-tss in that it uses the wrong internal NULL handle to try to create the NULL seed primary (and thus fails). Now that we're going to be using the NULL primary to salt sessions, the Intel TSS shim needs fixing to cope with thi correctly. The fix is to do the correct transform to the internal hierarchy representation on NULL hierarchy creation and to do the session handle conversion everywhere else. Additionally remove the intel_handle() code which was supposed to do this: it's unused because 0 is never passed in as a handle number. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- v2: reword commit message --- src/include/intel-tss.h | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) TSS_Delete(TSS_CONTEXT *tssContext) { @@ -937,8 +929,10 @@ tpm2_CreatePrimary(TSS_CONTEXT *tssContext, TPM_HANDLE primaryHandle, TPM2B_PUBLIC *opub; TPM_RC rc; - /* FIXME will generate wrong value for NULL hierarchy */ - primaryHandle = intel_handle(primaryHandle); + + /* TPM_RH_NULL is mapped to ESYS_TR_NONE, which won't work here */ + if (primaryHandle == TPM_RH_NULL) + primaryHandle = INT_TPM_RH_NULL; outsideInfo.size = 0; creationPcr.count = 0; @@ -993,9 +987,7 @@ tpm2_StartAuthSession(TSS_CONTEXT *tssContext, TPM_HANDLE tpmKey, TPM_HANDLE *sessionHandle, const char *bindPassword) { - bind = intel_handle(bind); - tpmKey = intel_handle(tpmKey); - if (bind != ESYS_TR_NONE) + if (bind != TPM_RH_NULL) intel_auth_helper(tssContext, bind, bindPassword); return Esys_StartAuthSession(tssContext, tpmKey, bind, ESYS_TR_NONE,