tpm: don't return -EINVAL if TPM command validation fails

This patch is an RFC because I'm not sure if this is the correct way to fix this issue. I'm not that familiar with the TPM driver so may had missed some details.

And example of user-space getting confused by the TPM chardev returning -EINVAL when sending a not supported TPM command can be seen in this tpm2-tools issue:

https://github.com/intel/tpm2-tools/issues/621

Quoted 3 lines · Javier Martinez Canillas

This patch is an RFC because I'm not sure if this is the correct way to fix this issue. I'm not that familiar with the TPM driver so may had missed some details.

Quoted 4 lines

And example of user-space getting confused by the TPM chardev returning -EINVAL when sending a not supported TPM command can be seen in this tpm2-tools issue:

https://github.com/intel/tpm2-tools/issues/621

I think this is a user space bug, unfortunately.

We talked about this when the spaces code was first written and it seemed the best was to just return EINVAL to indicate that the kernel could not accept the request.

This result is semantically different from the TPM could not execute or complete the request.

Regarding your specific issue, can you make the command you want to use validate? Would that make sense?

Yes, the problem with that is user-space not having enough information about what went wrong. Right now the TCTI layer just reports TSS2_BASE_RC_IO_ERROR in this case and can't be blamed.

Quoted 2 lines

Regarding your specific issue, can you make the command you want to use validate? Would that make sense?

Sorry, I'm not sure to understand what you meant. Could you please elaborate?

Quoted 3 lines · Javier Martinez Canillas

Yes, the problem with that is user-space not having enough information about what went wrong. Right now the TCTI layer just reports TSS2_BASE_RC_IO_ERROR in this case and can't be blamed.

Well, if you care about the differnce between a transport failure and a kernel rejection due to validation, then it needs to report a different code :)

Quoted 4 lines

Quoted 2 lines

Regarding your specific issue, can you make the command you want to use validate? Would that make sense?

Sorry, I'm not sure to understand what you meant. Could you please elaborate?

Make it so tpm_validate will accept the command being sent.

Jason

Right, that's what I understood indeed but wanted to be sure. The problem with that approach is that would not scale.

Since this particular TPM2 doesn't have support for the TPM2_EncryptDecrypt2 command, but some chips may not support others commands.

Quoted 5 lines · Javier Martinez Canillas

Right, that's what I understood indeed but wanted to be sure. The problem with that approach is that would not scale.

Since this particular TPM2 doesn't have support for the TPM2_EncryptDecrypt2 command, but some chips may not support others commands.

No, tpm_validate is not supposed to be sensitive to what commands the TPM supports. It is only supposed to check if the command passed is fully understood by the kernel and is properly formed.

This is to prevent rouge user space from sending garbage or privileged commands to the TPM.

If it is refusing TPM2_EncryptDecrypt2, and that command is safe to use in the spaces system, then tpm_validate must learn how to handle it, or userspace can never use it.

Jason

-----Original Message-----
From: Javier Martinez Canillas [mailto:[email protected]]
Sent: Friday, November 17, 2017 10:35 AM
To: Jason Gunthorpe <[email protected]>
Cc: [email protected]; Jarkko Sakkinen <[email protected]>; Peter Huewe <[email protected]>; Tricca, Philip B <[email protected]>; [email protected]; Roberts, William C <[email protected]>
Subject: Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

Quoted 7 lines · Jason Gunthorpe

Quoted 5 lines · Javier Martinez Canillas

Right, that's what I understood indeed but wanted to be sure. The problem with that approach is that would not scale.

Since this particular TPM2 doesn't have support for the TPM2_EncryptDecrypt2 command, but some chips may not support others

commands.

Quoted 5 lines

No, tpm_validate is not supposed to be sensitive to what commands the TPM supports. It is only supposed to check if the command passed is fully understood by the kernel and is properly formed.

Quoted 7 lines

This is to prevent rouge user space from sending garbage or privileged commands to the TPM.

If it is refusing TPM2_EncryptDecrypt2, and that command is safe to use in the spaces system, then tpm_validate must learn how to handle it, or userspace can never use it.

I see, misunderstood what the check was about then. If that's the case, then Making tpm_validate to learn how to check that command makes sense indeed and so does the -EINVAL error code.

Since it doesn't mean that the TPM doesn't support the command but just that the TPM spaces doesn't know how to handle it.

Need to look at the code in more detail, thanks a lot for the clarification.

Quoted 2 lines

Jason

Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement Red Hat

I don't know why spaces would filter by command code. But it does seem to be loaded By getting the command codes from the tpm in tpm2_get_tpm_pt().

I don't think that it's the right fix. I don't think the in-kernel RM should be filtering, but please enlighten my ignorance. Phillip did the userspace RM and understand the RM issues way better than I.

Quoted 3 lines · William C

I don't know why spaces would filter by command code. But it does seem to be loaded By getting the command codes from the tpm in tpm2_get_tpm_pt().

Ah, I forgot. So my remark is not quite right :\

Quoted 3 lines

I don't think that it's the right fix. I don't think the in-kernel RM should be filtering, but please enlighten my ignorance. Phillip did the userspace RM and understand the RM issues way better than I.

It needs to prevent unauthorized stuff from being sent to the TPM, so if the kernel does not know how to parse the command it shouldn't send it. It is a matter of security..

I can't remember if we synthezied responses for anything else, it could make sense to return the usual not supported command response for this specific thing. But the length error should remain EINVAL I think..

Jason

What I fail to understand is why that's not a problem when the TPM spaces infrastructure isn't used, tpm_validate_command() function just returns true if space is NULL. So when sending command to /dev/tpm0 directly, a rogue user-space program can send any arbitrary data to the TPM.

Quoted 4 lines · Javier Martinez Canillas

What I fail to understand is why that's not a problem when the TPM spaces infrastructure isn't used, tpm_validate_command() function just returns true if space is NULL. So when sending command to /dev/tpm0 directly, a rogue user-space program can send any arbitrary data to the TPM.

tpm spaces was designed to allow unprivileged user space access to

the TPM so it has additional protection designed to keep the TPM secure.

Allowing unprivileged user space to send send garbage to the TPM seems like a good way to trigger a TPM bug in parsing.

When spaces are not used /dev/tpm0 is root only and has full unrestricted access.

Jason

-----Original Message-----
From: Javier Martinez Canillas [mailto:[email protected]]
Sent: Monday, November 20, 2017 1:26 AM
To: Jason Gunthorpe <[email protected]>
Cc: Roberts, William C <[email protected]>; linux- [email protected]; Jarkko Sakkinen <[email protected]>; Peter Huewe <[email protected]>; Tricca, Philip B <[email protected]>; [email protected]
Subject: Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Javier Martinez Canillas

What I fail to understand is why that's not a problem when the TPM spaces infrastructure isn't used, tpm_validate_command() function just returns true if space is NULL. So when sending command to /dev/tpm0 directly, a rogue user-space program can send any arbitrary data to

the TPM.

Quoted 2 lines

tpm spaces was designed to allow unprivileged user space access to

Ah, I didn't know about that design decision. This isn't documented anywhere AFAICT, it would be nice to add some notes to Documentation/security/tpm/ so people are aware of this.

Quoted 2 lines

the TPM so it has additional protection designed to keep the TPM secure.

Quoted 4 lines

Allowing unprivileged user space to send send garbage to the TPM seems like a good way to trigger a TPM bug in parsing.

Well, I don't think that unprivileged user-space should have any access to the TPM. Since a rogue user-space can abuse the TPM anyway even when using a well constructed command (which is the only validation that's done IIUC).

In other words, only trusted software should have access to the TPM device.

I thought the TPM spaces was about exposing a virtualized TPM that didn't have the limitation of only allowing to store a small set of transient objects (so user- space didn't have to deal with the handles flushing and context save/load), rather than relaxing the access control to the TPM.

Quoted 2 lines

When spaces are not used /dev/tpm0 is root only and has full unrestricted access.

Quoted 1 line

The Linux motto is that it should provide mechanisms and not policy, so I wonder if is up to user-space to decide who should access the devices.

For example on my Fedora machine, the /dev/tpm* char devices are owned by the "tss" user and group. That's because the tpm2-abrmd package installs an udev rule to change the permissions, since the resource manager is run as the unprivileged tss user.

The /dev/tpmrm* on the other hand are owned by root. So on this distro at least, is the opposite of what you described.

Having said that, I'm happy to implement the synthesized response when the command is not supported, if that's the correct way to resolve this.

Quoted 2 lines

Jason

Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement Red Hat

That's policy, and shouldn't be hardcoded in the kernel. Let the DAC permission model And LSMs sort that out.

I thought the TPM spaces was about exposing a virtualized TPM that didn't have the limitation of only allowing to store a small set of transient objects (so user-space didn't have to deal with the handles flushing and context save/load), rather than relaxing the access control to the TPM.

Having said that, I'm happy to implement the synthesized response when the command is not supported, if that's the correct way to resolve this.

Quoted 5 lines · Javier Martinez Canillas

Right, that's what I understood indeed but wanted to be sure. The problem with that approach is that would not scale.

Since this particular TPM2 doesn't have support for the TPM2_EncryptDecrypt2 command, but some chips may not support others commands.

No, tpm_validate is not supposed to be sensitive to what commands the TPM supports. It is only supposed to check if the command passed is fully understood by the kernel and is properly formed.

This is to prevent rouge user space from sending garbage or privileged commands to the TPM.

If it is refusing TPM2_EncryptDecrypt2, and that command is safe to use in the spaces system, then tpm_validate must learn how to handle it, or userspace can never use it.

Do you really want to build in an every expanding list of commands that the kernel has to screen for? I don't think so.

Remember that there are new commands, optional commands, and vendor proprietary commands. What's the rationale for only looking at the command code and not rest of the parameters?

Quoted 7 lines · Javier Martinez Canillas

This patch is an RFC because I'm not sure if this is the correct way to fix this issue. I'm not that familiar with the TPM driver so may had missed some details.

And example of user-space getting confused by the TPM chardev returning -EINVAL when sending a not supported TPM command can be seen in this tpm2-tools issue:

https://github.com/intel/tpm2-tools/issues/621

I think this is a user space bug, unfortunately.

We talked about this when the spaces code was first written and it seemed the best was to just return EINVAL to indicate that the kernel could not accept the request.

This result is semantically different from the TPM could not execute or complete the request.

According to the TPM Library Specification, a TPM device must do a command header validation before processing and return a TPM_RC_COMMAND_CODE code if the command is not implemented and the TPM_RC_COMMAND_SIZE code if the command buffer size is not correct.

So user-space will expect to handle these response codes as errors, but if the in-kernel resource manager is used (/dev/tpmrm?) then an -EINVAL errno code is returned instead if the command isn't implemented or the buffer size isn't correct. This confuses user-space since doesn't expect that.

This is also not consistent with the behavior when not using TPM spaces and accessing the TPM directly (/dev/tpm?), in this case the command is is sent to the TPM anyways and user-space can get an error from the TPM.

Instead of returning an -EINVAL errno code when the tpm_validate_command() function fails, allow the command to be sent to the TPM but just don't do any TPM space management. That way the TPM can report back a proper error and the behavior be consistent when using either /dev/tpm? or /dev/tpmrm?.

Signed-off-by Javier Martinez Canillas <[email protected]>

Quoted 14 lines · Javier Martinez Canillas

According to the TPM Library Specification, a TPM device must do a command header validation before processing and return a TPM_RC_COMMAND_CODE code if the command is not implemented and the TPM_RC_COMMAND_SIZE code if the command buffer size is not correct.

So user-space will expect to handle these response codes as errors, but if the in-kernel resource manager is used (/dev/tpmrm?) then an -EINVAL errno code is returned instead if the command isn't implemented or the buffer size isn't correct. This confuses user-space since doesn't expect that.

This is also not consistent with the behavior when not using TPM spaces and accessing the TPM directly (/dev/tpm?), in this case the command is is sent to the TPM anyways and user-space can get an error from the TPM.

Quoted 6 lines

Instead of returning an -EINVAL errno code when the tpm_validate_command() function fails, allow the command to be sent to the TPM but just don't do any TPM space management. That way the TPM can report back a proper error and the behavior be consistent when using either /dev/tpm? or /dev/tpmrm?.

Signed-off-by Javier Martinez Canillas <[email protected]>

It is not a virtual TPM so I don't think that matters. It at least

matters less than breaking the sandbox.

/Jarkko

Quoted 2 lines · Jarkko Sakkinen

matters less than breaking the sandbox.

Yes, sorry for that. It wasn't clear to me that there was a sandbox and my lack of familiarity with the code was the reason why I posted as a RFC in the first place.

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

As mentioned, I think this should be documented. I guess most people would see the in-kernel resource manager as a virtualized TPM, since the "TSS TAB and Resource Manager Specification" [0] explains the RM making an analogy with a virtual memory manager:

"The Resource Manager (RM) manages the TPM context in a manner similar to a virtual memory manager. It swaps objects, sessions, and sequences in and out of the limited TPM memory as needed."

And even your latest LPC presentation mention that the handles in the in-kernel resource manager are virtualized [1].

And I disagree that it does not matter, since the same spec says:

"This layer is mostly transparent to the upper layers of the TSS and is not required."

But returning -EINVAL instead of a proper TPM command response with a TPM_RC_COMMAND_CODE code makes it not transparent to the upper layer.

If the TPM spaces infrastructure is not compliant with the spec, then I think that should also be documented.

Quoted 2 lines

matters less than breaking the sandbox.

Yes, sorry for that. It wasn't clear to me that there was a sandbox and my lack of familiarity with the code was the reason why I posted as a RFC in the first place.

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Quoted 8 lines · Javier Martinez Canillas

As mentioned, I think this should be documented. I guess most people would see the in-kernel resource manager as a virtualized TPM, since the "TSS TAB and Resource Manager Specification" [0] explains the RM making an analogy with a virtual memory manager:

"The Resource Manager (RM) manages the TPM context in a manner similar to a virtual memory manager. It swaps objects, sessions, and sequences in and out of the limited TPM memory as needed."

A process in virtual memory has a different environment than code running on bare metal without page table, doesn't it?

Quoted 5 lines

And even your latest LPC presentation mention that the handles in the in-kernel resource manager are virtualized [1].

And I disagree that it does not matter, since the same spec says:

Quoted 5 lines

"This layer is mostly transparent to the upper layers of the TSS and is not required."

But returning -EINVAL instead of a proper TPM command response with a TPM_RC_COMMAND_CODE code makes it not transparent to the upper layer.

*mostly*

Quoted 2 lines

If the TPM spaces infrastructure is not compliant with the spec, then I think that should also be documented.

TPM specification is not a formal specification AFAIK.

Quoted 3 lines

Quoted 2 lines

matters less than breaking the sandbox.

Quoted 6 lines

Yes, sorry for that. It wasn't clear to me that there was a sandbox and my lack of familiarity with the code was the reason why I posted as a RFC in the first place.

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Nope.

/Jarkko

Quoted 23 lines

Quoted 8 lines · Javier Martinez Canillas

As mentioned, I think this should be documented. I guess most people would see the in-kernel resource manager as a virtualized TPM, since the "TSS TAB and Resource Manager Specification" [0] explains the RM making an analogy with a virtual memory manager:

"The Resource Manager (RM) manages the TPM context in a manner similar to a virtual memory manager. It swaps objects, sessions, and sequences in and out of the limited TPM memory as needed."

A process in virtual memory has a different environment than code running on bare metal without page table, doesn't it?

Quoted 9 lines

And even your latest LPC presentation mention that the handles in the in-kernel resource manager are virtualized [1].

And I disagree that it does not matter, since the same spec says:

"This layer is mostly transparent to the upper layers of the TSS and is not required."

Quoted 6 lines

Quoted 3 lines

But returning -EINVAL instead of a proper TPM command response with a TPM_RC_COMMAND_CODE code makes it not transparent to the upper layer.

*mostly*

Fair enough

Quoted 8 lines

Quoted 2 lines

If the TPM spaces infrastructure is not compliant with the spec, then I think that should also be documented.

TPM specification is not a formal specification AFAIK.

Quoted 3 lines

Quoted 2 lines

matters less than breaking the sandbox.

Quoted 9 lines

Quoted 6 lines

Yes, sorry for that. It wasn't clear to me that there was a sandbox and my lack of familiarity with the code was the reason why I posted as a RFC in the first place.

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Nope.

Ok. Thanks a lot for your feedback. I already had that patch but didn't want to post it before knowing your opinion, I'll drop it now.

Philip,

I think this means that we can now fix this in user-space then? That was in fact my first suggestion in the filed tpm2-tools issue.

Apologies for the slow response. I didn't get switched over from tpmdd-devel to linux-integrity till just now.

Quoted 24 lines · Jarkko Sakkinen

Quoted 23 lines

Quoted 8 lines · Javier Martinez Canillas

As mentioned, I think this should be documented. I guess most people would see the in-kernel resource manager as a virtualized TPM, since the "TSS TAB and Resource Manager Specification" [0] explains the RM making an analogy with a virtual memory manager:

"The Resource Manager (RM) manages the TPM context in a manner similar to a virtual memory manager. It swaps objects, sessions, and sequences in and out of the limited TPM memory as needed."

A process in virtual memory has a different environment than code running on bare metal without page table, doesn't it?

Quoted 9 lines

And even your latest LPC presentation mention that the handles in the in-kernel resource manager are virtualized [1].

And I disagree that it does not matter, since the same spec says:

"This layer is mostly transparent to the upper layers of the TSS and is not required."

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

But returning -EINVAL instead of a proper TPM command response with a TPM_RC_COMMAND_CODE code makes it not transparent to the upper layer.

*mostly*

Fair enough

The intent of this "mostly transparent" stuff is to convey that the RM should be as transparent as possible while acknowledging that there are some cases where it's not / can't be. I can't say why the original author phrased it in this somewhat ambiguous way but I wouldn't call this a fair interpretation. It's definitely one way to read it though.

The case in question is the RM performing a function on behalf of the
TPM: command code validation. This is a perfectly valid thing to do in the RM though the RM should aim to behave as the TPM would if the RM takes any action (sending a TPM response buffer with the appropriate response code).

An additional detail is described in section 3.1 "Error Codes". There is a mechanism to encode information about which layer in the stack produced the response buffer. When the TPM gets a command with a command code it doesn't support then this field will be '0' since '0' identifies the TPM. If the RM is taking over this function it should set the field to indicate as much.

Quoted 22 lines

Quoted 8 lines

Quoted 2 lines

If the TPM spaces infrastructure is not compliant with the spec, then I think that should also be documented.

TPM specification is not a formal specification AFAIK.

Quoted 3 lines

Quoted 2 lines

matters less than breaking the sandbox.

Quoted 9 lines

Quoted 6 lines

Yes, sorry for that. It wasn't clear to me that there was a sandbox and my lack of familiarity with the code was the reason why I posted as a RFC in the first place.

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Nope.

Ok. Thanks a lot for your feedback. I already had that patch but didn't want to post it before knowing your opinion, I'll drop it now.

Quoted 4 lines

Philip,

I think this means that we can now fix this in user-space then? That was in fact my first suggestion in the filed tpm2-tools issue.

We can work around quirks in the kernel RM in user space if we must (short term?) but I'm hesitant to do so in this case. Would feel better about a short term work-around knowing it's only going to be short term.

Philip

That was my interpretation as well and what I was arguing about. I'm glad to know that you also think the same.

Quoted 2 lines · Javier Martinez Canillas

That was my interpretation as well and what I was arguing about. I'm glad to know that you also think the same.

It could be that this rationale has been your earlier emails but I just haven't recognized it :-) I think I'm starting to buy this.

I don't have any fixed standing points anything basically. It is just better to be really resistant with anything that is related to user-kernel interaction until you really get it...

/Jarkko

We can work around quirks in the kernel RM in user space if we must (short term?) but I'm hesitant to do so in this case. Would feel better about a short term work-around knowing it's only going to be short term.

Quoted 3 lines · flihp

We can work around quirks in the kernel RM in user space if we must (short term?) but I'm hesitant to do so in this case. Would feel better about a short term work-around knowing it's only going to be short term.

Pedantically, the kernel is not implementing a RM as per some spec, it is using the TPM features to create isolation.

Both sides can be argued. In this case, I think the patch to add a TPM response buffer in this one case is reasonable, and does not look so complicated that it is dangerous in the kernel.

However the kernel will continue to return errnos in various cases, and a userspace that cannot handle them is kinda broken :)

First, to handle the error, the user space TSS would have to know that the RM is mapping what would normally (with the simulator or /dev/tpm0) be the usual TPM response packet. This mapping isn't documented anywhere.

The intent of this "mostly transparent" stuff is to convey that the RM should be as transparent as possible while acknowledging that there are some cases where it's not / can't be. I can't say why the original author phrased it in this somewhat ambiguous way but I wouldn't call this a fair interpretation. It's definitely one way to read it though.

The case in question is the RM performing a function on behalf of the
TPM: command code validation. This is a perfectly valid thing to do in the RM though the RM should aim to behave as the TPM would if the RM takes any action (sending a TPM response buffer with the appropriate response code).

An additional detail is described in section 3.1 "Error Codes". There is a mechanism to encode information about which layer in the stack produced the response buffer. When the TPM gets a command with a command code it doesn't support then this field will be '0' since '0' identifies the TPM. If the RM is taking over this function it should set the field to indicate as much.

Quoted 12 lines · flihp

The intent of this "mostly transparent" stuff is to convey that the RM should be as transparent as possible while acknowledging that there are some cases where it's not / can't be. I can't say why the original author phrased it in this somewhat ambiguous way but I wouldn't call this a fair interpretation. It's definitely one way to read it though.

The case in question is the RM performing a function on behalf of the
TPM: command code validation. This is a perfectly valid thing to do in the RM though the RM should aim to behave as the TPM would if the RM takes any action (sending a TPM response buffer with the appropriate response code).

Quoted 6 lines

An additional detail is described in section 3.1 "Error Codes". There is a mechanism to encode information about which layer in the stack produced the response buffer. When the TPM gets a command with a command code it doesn't support then this field will be '0' since '0' identifies the TPM. If the RM is taking over this function it should set the field to indicate as much.

Thanks for explaining this. I guess we could take this direction. I think by utilizing the field that you mentioned we could consider this. And it would be hard to imagine this change to cause anything serious (if anything at all) with backwards compatbility.

Javier, does you current version use this field? If not can you resend an update?

/Jarkko

Ok. Thanks a lot for your feedback. I already had that patch but didn't want to post it before knowing your opinion, I'll drop it now.

Philip,

I think this means that we can now fix this in user-space then? That was in fact my first suggestion in the filed tpm2-tools issue.

-----Original Message-----
From: Jarkko Sakkinen [mailto:[email protected]]
Sent: Tuesday, November 21, 2017 4:30 AM
To: Javier Martinez Canillas <[email protected]>
Cc: [email protected]; Peter Huewe <[email protected]>; Tricca, Philip B <[email protected]>; Jason Gunthorpe <[email protected]>; [email protected]; Roberts, William C <[email protected]>
Subject: Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

Quoted 8 lines · Javier Martinez Canillas

As mentioned, I think this should be documented. I guess most people would see the in-kernel resource manager as a virtualized TPM, since the "TSS TAB and Resource Manager Specification" [0] explains the RM making an analogy with a virtual memory manager:

"The Resource Manager (RM) manages the TPM context in a manner similar to a virtual memory manager. It swaps objects, sessions, and sequences in and out of the limited TPM memory as needed."

A process in virtual memory has a different environment than code running on bare metal without page table, doesn't it?

Quoted 10 lines

And even your latest LPC presentation mention that the handles in the in-kernel resource manager are virtualized [1].

And I disagree that it does not matter, since the same spec says:

"This layer is mostly transparent to the upper layers of the TSS and is not required."

But returning -EINVAL instead of a proper TPM command response with a TPM_RC_COMMAND_CODE code makes it not transparent to the upper layer.

*mostly*

Quoted 2 lines

If the TPM spaces infrastructure is not compliant with the spec, then I think that should also be documented.

TPM specification is not a formal specification AFAIK.

Quoted 4 lines

Quoted 3 lines

matters less than breaking the sandbox.

Quoted 6 lines

Yes, sorry for that. It wasn't clear to me that there was a sandbox and my lack of familiarity with the code was the reason why I posted as a RFC in the first place.

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Nope.

/Jarkko

Quoted 5 lines

Quoted 3 lines

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Nope.

We should update the elf loader to make sure that ELF files don't contain Incorrect instructions. We shouldn't have this type of policy in the driver considering that the tpm is designed to handle it. Obviously you disagree, just understand you're wrong :-P

On 11/21/2017 09:29 PM, Roberts, William C wrote:

[snip]

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Nope.

We should update the elf loader to make sure that ELF files don't contain Incorrect instructions. We shouldn't have this type of policy in the driver considering that the tpm is designed to handle it. Obviously you disagree, just understand you're wrong :-P

I think the sandbox is correct and makes sense to only send well constructed commands to the TPM. So my RFC patch breaking the sandbox is clearly wrong.

I still do believe that both interfaces (/dev/tpm and /dev/tpmrm) should be consistent if possible though. In other words, I don't see the value of not behaving as expected by the spec if this doesn't have security implications as is the case with the approach suggested by Jason. And the implementation for sending the synthesized response is also trivial.

The other option that's fixing this in user-space will be a workaround, since it would either be to check for TPM_RC_SUCCESS instead of TPM_RC_COMMAND_CODE or make the SAPI library infer that a -EINVAL error means that a command isn't supported and return a TPM_RC_COMMAND_CODE to the caller.

For completeness, I'll share my patch implementing what Jason suggested, even when is quite likely that Jarkko won't like it since he has a strong opinion on this:

From 145b6891a68b32ae803a4b0abd3d35679ed6b2a1 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <[email protected]>
Date: Tue, 21 Nov 2017 12:32:15 +0100
Subject: [RFCv2 PATCH] tpm: return a TPM_RC_COMMAND_CODE response if command
isn't implemented

According to the TPM Library Specification, a TPM device must do a command header validation before processing and return a TPM_RC_COMMAND_CODE code if the command is not implemented.

So user-space will expect to handle the response cods as error. But if the in-kernel resource manager is used (/dev/tpmrm?), an -EINVAL errno code is returned instead if the command isn't implemented. This confuses userspace since it doesn't expect that error value.

This also isn't consistent with the behavior when not using TPM spaces and accessing the TPM directly (/dev/tpm?). In this case, the command is sent to the TPM even when implemented and userspace gets an error from the TPM.

Instead of returning an -EINVAL errno code when the tpm_validate_command() function fails, synthesize a TPM command response so userspace can get a TPM_RC_COMMAND_CODE as expected when a chip doesn't implement the command.

Suggested-by Jason Gunthorpe <[email protected]>

Signed-off-by Javier Martinez Canillas <[email protected]>

---

Changes since RFCv1:
- Don't send not validated commands to the TPM, instead return a synthesized response with the correct TPM return code (suggested by Jason Gunthorpe).

 drivers/char/tpm/tpm-interface.c | 28 ++++++++++++++++++++--------
 drivers/char/tpm/tpm.h           |  1 +
 2 files changed, 21 insertions(+), 8 deletions(-)

drivers/char/tpm/tpm-interface.c +20 -8

index ebe0a1d36d8c..b8d01897c0ba 100644
@@ -328,7 +328,7 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip,
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
-static bool tpm_validate_command(struct tpm_chip *chip,
+static int tpm_validate_command(struct tpm_chip *chip,
 				 struct tpm_space *space,
 				 const u8 *cmd,
 				 size_t len)

@@ -340,10 +340,10 @@ static bool tpm_validate_command(struct tpm_chip *chip,
 	unsigned int nr_handles;
 
 	if (len < TPM_HEADER_SIZE)
-		return false;
+		return -EINVAL;
 
 	if (!space)
-		return true;
+		return 0;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2 && chip->nr_commands) {
 		cc = be32_to_cpu(header->ordinal);

@@ -352,7 +352,7 @@ static bool tpm_validate_command(struct tpm_chip *chip,
 		if (i < 0) {
 			dev_dbg(&chip->dev, "0x%04X is an invalid command\n",
 				cc);
-			return false;
+			return -EOPNOTSUPP;
 		}
 
 		attrs = chip->cc_attrs_tbl[i];

@@ -362,11 +362,11 @@ static bool tpm_validate_command(struct tpm_chip *chip,
 			goto err_len;
 	}
 
-	return true;
+	return 0;
 err_len:
 	dev_dbg(&chip->dev,
 		"%s: insufficient command length %zu", __func__, len);
-	return false;
+	return -EINVAL;
 }
 
 /**

@@ -391,8 +391,20 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	unsigned long stop;
 	bool need_locality;
 
-	if (!tpm_validate_command(chip, space, buf, bufsiz))
-		return -EINVAL;
+	rc = tpm_validate_command(chip, space, buf, bufsiz);
+	if (rc == -EINVAL)
+		return rc;
+	/*
+	 * If the command is not implemented by the TPM, synthesize a
+	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
+	 */
+	if (rc == -EOPNOTSUPP) {
+		header->length = cpu_to_be32(sizeof(*header));
+		header->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+		header->return_code = cpu_to_be32(TPM2_RC_COMMAND_CODE);
+
+		return bufsiz;
+	}
 
 	if (bufsiz > TPM_BUFSIZE)
 		bufsiz = TPM_BUFSIZE;

drivers/char/tpm/tpm.h +1 -0

index c1866cc02e30..40818fa59b05 100644
@@ -100,6 +100,7 @@ enum tpm2_return_codes {
 	TPM2_RC_HANDLE		= 0x008B,
 	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
 	TPM2_RC_DISABLED	= 0x0120,
+	TPM2_RC_COMMAND_CODE    = 0x0143,
 	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
 	TPM2_RC_REFERENCE_H0	= 0x0910,
 };

Signature · 1 line

2.14.3

Quoted 30 lines · Javier Martinez Canillas

On 11/21/2017 09:29 PM, Roberts, William C wrote:

[snip]

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Nope.

We should update the elf loader to make sure that ELF files don't contain Incorrect instructions. We shouldn't have this type of policy in the driver considering that the tpm is designed to handle it. Obviously you disagree, just understand you're wrong :-P

I think the sandbox is correct and makes sense to only send well constructed commands to the TPM. So my RFC patch breaking the sandbox is clearly wrong.

I still do believe that both interfaces (/dev/tpm and /dev/tpmrm) should be consistent if possible though. In other words, I don't see the value of not behaving as expected by the spec if this doesn't have security implications as is the case with the approach suggested by Jason. And the implementation for sending the synthesized response is also trivial.

The other option that's fixing this in user-space will be a workaround, since it would either be to check for TPM_RC_SUCCESS instead of TPM_RC_COMMAND_CODE or make the SAPI library infer that a -EINVAL error means that a command isn't supported and return a TPM_RC_COMMAND_CODE to the caller.

Quoted 3 lines

For completeness, I'll share my patch implementing what Jason suggested, even when is quite likely that Jarkko won't like it since he has a strong opinion on this:

I apologize for long delay. I have this enormous upstreaming project on my shoulders [1], which will temporarily cause more delay for TPM but things will settle once it is pulled to the mainline.

I'll go through the patch within next few days.

[1] https://lkml.org/lkml/2017/11/25/123

/Jarkko

I still do believe that both interfaces (/dev/tpm and /dev/tpmrm) should be consistent if possible though.

In other words, I don't see the value of not
behaving as expected by the spec if this doesn't have security implications as is the case with the approach suggested by Jason. And the implementation for sending the synthesized response is also trivial.

The other option that's fixing this in user-space will be a workaround, since it would either be to check for TPM_RC_SUCCESS instead of TPM_RC_COMMAND_CODE or make the SAPI library infer that a -EINVAL error means that a command isn't supported and return a TPM_RC_COMMAND_CODE to the caller.

Quoted 1 line

TPM specification is not a formal specification AFAIK.

The published parts are, granted many things are changing.

Quoted 7 lines

Quoted 6 lines

Yes, sorry for that. It wasn't clear to me that there was a sandbox and my lack of familiarity with the code was the reason why I posted as a RFC in the first place.

Do you agree with Jason's suggestion to send a synthesized TPM command in the that the command isn't supported?

Quoted 1 line

Nope.

We should update the elf loader to make sure that ELF files don't contain Incorrect instructions. We shouldn't have this type of policy in the driver considering that the tpm is designed to handle it. Obviously you disagree, just understand you're wrong :-P

I think -EINVAL is better than synthetizing commands that are not really from the TPM. And we would break backwards compatability by doing this.

As I said in an earlier response I would rather compare resource manager to virtual memory than virtual machine.

Quoted 6 lines · Jarkko Sakkinen

I think -EINVAL is better than synthetizing commands that are not really from the TPM. And we would break backwards compatability by doing this.

As I said in an earlier response I would rather compare resource manager to virtual memory than virtual machine.

Agreed that synthesizing a response is not trivial. (It's not that hard either - a 6 byte hard coded header and a 4 byte big endian integer.)

But what would be wrong with sending an unknown command to the TPM and letting it handle the response?

I agree with you. As I said in this thread, I don't understand the security implications that Jason says. The patch in $SUBJECT just avoids all the TPM spaces code paths and sends the command that user-space passed to the kernel to the real TPM2 hardware as it would do when writing to /dev/tpm? directly.

The kernel should provide mechanism and not policy in my opinion, so it should be up to user-space to decide what programs are allowed to access the chardevs or not. In any case, I'm also OK with the solution that was suggested and was merged.

Ken, Javier,

Here's my counterpart to you :-) Sorry for the delay. I'm quite busy upstreaming SGX ATM.

Quoted 5 lines

I agree with you. As I said in this thread, I don't understand the security implications that Jason says. The patch in $SUBJECT just avoids all the TPM spaces code paths and sends the command that user-space passed to the kernel to the real TPM2 hardware as it would do when writing to /dev/tpm? directly.

Quoted 4 lines

The kernel should provide mechanism and not policy in my opinion, so it should be up to user-space to decide what programs are allowed to access the chardevs or not. In any case, I'm also OK with the solution that was suggested and was merged.

I would say kernel should keep the amount of policy minimal. Your statement is a "textbook definition" what kernel should do. Sandbox

always requires some policy.

We do not have TPMA_CC entry for unknown command so we don't know how it might change the TPM state so obviously we must block such command and not let it through. There is no other option when doing an RM implementation.

If we ignore security implications, it can at least completely break the in-kernel RM state.

I did not really get the Ken's comment about incompatibility with different RM's. I guess all TPM user spaces should be able to handle TPM_RC_COMMAND_CODE and top bits are part of the TCG standard (TSS2_RESMGR_TPM_RC_LAYER):

https://trustedcomputinggroup.org/wp-content/uploads/TCG-TSS-2.0-Overview-and-Common-Structures-Specification-Version-0.90-Revision-02.pdf

/Jarkko

I did not really get the Ken's comment about incompatibility with different RM's. I guess all TPM user spaces should be able to handle TPM_RC_COMMAND_CODE and top bits are part of the TCG standard (TSS2_RESMGR_TPM_RC_LAYER):

Remember also that SAPI is just one TSS design. There are currently three others. And SAPI is targeted more as a building block than an end user library.

Every TSS implementation would have to do this mapping. How would they even know to do it if they didn't notice this thread? It wouldn't be documented anywhere other than deep in kernel code.

Quoted 4 lines · Jarkko Sakkinen

I think -EINVAL is better than synthetizing commands that are not really from the TPM. And we would break backwards compatability by doing this.

Quoted 2 lines

As I said in an earlier response I would rather compare resource manager to virtual memory than virtual machine.

Agreed that synthesizing a response is not trivial. (It's not that hard either - a 6 byte hard coded header and a 4 byte big endian integer.)

But what would be wrong with sending an unknown command to the TPM and letting it handle the response?

According to the TPM Library Specification, a TPM device must do a command header validation before processing and return a TPM_RC_COMMAND_CODE code if the command is not implemented and the TPM_RC_COMMAND_SIZE code if the command buffer size is not correct.

So user-space will expect to handle these response codes as errors, but if the in-kernel resource manager is used (/dev/tpmrm?) then an -EINVAL errno code is returned instead if the command isn't implemented or the buffer size isn't correct. This confuses user-space since doesn't expect that.

This is also not consistent with the behavior when not using TPM spaces and accessing the TPM directly (/dev/tpm?), in this case the command is is sent to the TPM anyways and user-space can get an error from the TPM.

Instead of returning an -EINVAL errno code when the tpm_validate_command() function fails, allow the command to be sent to the TPM but just don't do any TPM space management. That way the TPM can report back a proper error and the behavior be consistent when using either /dev/tpm? or /dev/tpmrm?.