Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Tue, Oct 06, 2015 at 06:22:29AM +0000, Fuchs, Andreas wrote:
> > > OK, I guess we got stuck in the follow-up discussions and missed the points.
> > 
> > Yup, don't get me wrong here. I like this discussion and am willing to
> > listen to reasonable arguments.
> 
> We could not agree more. I'm always up for a good discussion... ;-)
> 
> > > My 1st point is:
> > >
> > > TPM1.2's 0x40000000 SRK handle was a well-known, singleton, always-present
> > > key, that could be relied upon.
> > >
> > > TPM2.0's 0x80000000 is a temporary, TPM-assigned, context-specific handle,
> > > that cannot be relied upon.
> > >
> > > Therefore, I think your patch should not use it.
> > >
> > > Instead, I'd recommend using the closest equivalent to an SRK that TPM2.0
> > > has to offer, which is within the range 0x81000000 to 0x8100FFFF.
> > > (see http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> > > You might want to use TPM2_GetCapability() to find the correct one.
> > >
> > > Also User-Space could reference any of these handles in the
> > > 0x81000000-0x81FFFFFF range. This would be fine.
> > 
> > Alright. How about requiring keyhandle as explicit option for TPM 2.0?
> > Would that be a more reasonable solution in your opinion? That would
> > acceptable for me.
> 
> You mean getting rid of the default behaviour ?
> That sound reasonable to me as well. A later patch could add the possibility
> to use the TPM2_GetCapability() thing at a later stage then...
> 
> > > My 2nd point is:
> > >
> > > It is IMHO a bad idea to allow user-space to provide transient handles as
> > > parameter to the TPM, because TSS2.0 will virtualize handles and /dev/tpm0
> > > is single-access only.
> > > Instead I'd recommend passing context-saved-blobs to the kernel.
> > >
> > > Then you brought up the valid point that this requires kernel-space resource
> > > broker and I provided some sketch-idea in pseudo-code for discussion of
> > > general approach. I did not know that the access broker was solved already.
> > 
> > Yeah. I'm not against implementing the broker and how I've been thinking
> > implementing it is not too far away what you just suggested.
> > 
> > I'm not just seeing why that couldn't be done as a separate patch set
> > later on.
> 
> I should have been more clear then. I agree that it can be added later on.
> Or rather I think it should be added at some later point...
> 
> I was just trying to point out that the concept is not too difficult, since
> kernel-space (minimal) resource-manager makes a lot of people go "oh god,
> never ever, way too big, way too complicated", which IMHO it is not.
> Until then, I think that the call should just return -EBUSY when you cannot
> load the sealed data if no slots are available ?

Well this is kind of argument where there is no argument. I already had
plans how to do access broker back in 2014 that are more or less along
the lines of the pseudo code you sent. The problem is the lack of active
maintainers in the subsystem. That's why I get easily frustated
discussing about access broker in the first place :)

I would have implemented access broker months and months ago if I didn't
have so much code in the queue for this subsystem. There can be literally
months delay without any feedback. Right now I have four different
patches or patch sets in the queue:

- PPI support (yes you cannot enable TPM chips at the moment from Linux)
- Two bug fixes (CRB command buffer alignment, dTPM2 ACPI hot plugging)
- Basic trusted keys

I wouldn't blame any particular person about the situation but things
cannot continue like this. I've been thinking if I could acquire
co-maintainership of the subsystem for TPM 2 parts (don't really have
time or expertise for TPM 1.x parts).

I could post my architecture (never really written it except in my head
but should not take too long time) to my blog at jsakkine.blogspot.com
if you are interested discussing more.

> I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
> and TPM2_Load()-failures ?
> You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
> in those cases. Would you agree ?
> (P.S. I can cross-post there if that's prefered ?)

Have to check the return values. I posted this patch set already in
early July. You are the first reviewer in three months for this patch
set.

I think the reason was that for TPM 1.x returned -EPERM in all error
scenarios and I didn't want to endanger behaviour of command-line tools
such as 'keyctl'. I would keep it that way unless you can guarantee that
command-line tools will continue work correctly if I change it to
-EBUSY.

Anyway, I will recheck this part of the patch set but likely are not
going to do any changes because I don't want to break the user space.

I will consider revising the patch set with keyhandle required as an
explicit option.

> Cheers,
> Andreas

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/