diff mbox series

[v2,1/8] tss: Fix handling of TPM_RH_NULL in intel-tss

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

Commit Message

James Bottomley Aug. 4, 2024, 1:42 p.m. UTC
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,

Comments

James Bottomley Aug. 4, 2024, 3:37 p.m. UTC | #1
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;
Jarkko Sakkinen Aug. 4, 2024, 9:28 p.m. UTC | #2
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
James Bottomley Aug. 5, 2024, 2:48 a.m. UTC | #3
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
Jarkko Sakkinen Aug. 5, 2024, 11:54 a.m. UTC | #4
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 mbox series

Patch

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