| Message ID | 20171117100724.19257-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 17, 2017 at 11:07:24AM +0100, Javier Martinez Canillas wrote: > 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. Regarding your specific issue, can you make the command you want to use validate? Would that make sense? > + /* > + * If command validation fails, sent it to the TPM anyways so it can > + * report a proper error to user-space. Just don't do any TPM space > + * management in this case. > + */ > + cmd_validated = tpm_validate_command(chip, space, buf, bufsiz); And sending a command that failed to validate to the TPM cannot be done, as it violates our security model Jason
Hello Jason, Thanks a lot for your feedback. On 11/17/2017 05:57 PM, Jason Gunthorpe wrote: > On Fri, Nov 17, 2017 at 11:07:24AM +0100, Javier Martinez Canillas wrote: > >> 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. > No worries, as mentioned I posted this RFC mostly to raise awareness of the issue and to get feedback on how it could be properly solved. > 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. > 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. Maybe Philip can comment how this could be handled in user-space since he has a much better understanding of the TCTI and SAPI layers. > 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? Best regards,
On Fri, Nov 17, 2017 at 06:56:09PM +0100, Javier Martinez Canillas wrote: > 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 :) > > 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
On 11/17/2017 06:58 PM, Jason Gunthorpe wrote: > On Fri, Nov 17, 2017 at 06:56:09PM +0100, Javier Martinez Canillas wrote: > >> 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 :) > Fair enough, the hard part I guess would be to decide which errno codes to use that could better map to the actual TPM_RC_COMMAND_{CODE,SIZE} response codes. I'll give some thought to this and also discuss with the tpm2 tools/tss folks. >>> 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. > 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. So I rather prefer to have a consistent way for the kernel to report when a command is found to not be supported and user-space to understand it. > Jason > Best regards,
On Fri, Nov 17, 2017 at 07:10:09PM +0100, Javier Martinez Canillas wrote: > 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
On 11/17/2017 07:17 PM, Jason Gunthorpe wrote: > On Fri, Nov 17, 2017 at 07:10:09PM +0100, Javier Martinez Canillas wrote: > >> 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. > 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. > Jason > Best regards,
> -----Original Message----- > From: Javier Martinez Canillas [mailto:javierm@redhat.com] > Sent: Friday, November 17, 2017 10:35 AM > To: Jason Gunthorpe <jgg@ziepe.ca> > Cc: linux-kernel@vger.kernel.org; Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com>; Peter Huewe <peterhuewe@gmx.de>; > Tricca, Philip B <philip.b.tricca@intel.com>; linux-integrity@vger.kernel.org; > Roberts, William C <william.c.roberts@intel.com> > Subject: Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation > fails > > On 11/17/2017 07:17 PM, Jason Gunthorpe wrote: > > On Fri, Nov 17, 2017 at 07:10:09PM +0100, Javier Martinez Canillas wrote: > > > >> 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. > > > > 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. Yeah it looks like it would fail in tpm2_find_cc 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(). So even if someone Is using spaces and the tpm is some development model with crazy commands, they Should get loaded into the command list. We could implement similar logic in the userspace tools where the issue is occurring. 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. I just don't like the fact that it issues a different return code, the userspace RM doesn't do this. If a command isn't supported it should issue a TPM response saying so, it's virtualizing a tpm. > > Need to look at the code in more detail, thanks a lot for the clarification. > > > Jason > > > > Best regards, > -- > Javier Martinez Canillas > Software Engineer - Desktop Hardware Enablement Red Hat
On Fri, Nov 17, 2017 at 07:14:21PM +0000, Roberts, William C wrote: > 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 :\ > 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
On 11/18/2017 12:55 AM, Jason Gunthorpe wrote: > On Fri, Nov 17, 2017 at 07:14:21PM +0000, Roberts, William C wrote: > >> 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 :\ > Right, so it seems I didn't completely misunderstand the code after all :) >> 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.. > 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. And also according to the TCG spec, the TPM should validate the command header before it attempts to process the command. > 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 > Best regards,
On Sat, Nov 18, 2017 at 01:53:49AM +0100, Javier Martinez Canillas wrote: > 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
On 11/19/2017 04:27 PM, Jason Gunthorpe wrote: > On Sat, Nov 18, 2017 at 01:53:49AM +0100, Javier Martinez Canillas wrote: > >> 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 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. > 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. > 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. > When spaces are not used /dev/tpm0 is root only and has full > unrestricted access. > 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. > Jason > Best regards,
> -----Original Message----- > From: Javier Martinez Canillas [mailto:javierm@redhat.com] > Sent: Monday, November 20, 2017 1:26 AM > To: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Roberts, William C <william.c.roberts@intel.com>; linux- > kernel@vger.kernel.org; Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; > Peter Huewe <peterhuewe@gmx.de>; Tricca, Philip B > <philip.b.tricca@intel.com>; linux-integrity@vger.kernel.org > Subject: Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation > fails > > On 11/19/2017 04:27 PM, Jason Gunthorpe wrote: > > On Sat, Nov 18, 2017 at 01:53:49AM +0100, Javier Martinez Canillas wrote: > > > >> 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 Shouldn't it depend on the mode of the device file? > > 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. > > > the TPM so it has additional protection designed to keep the TPM > > secure. The TPM manages its own security over objects, can you give me an example of where this matters? I see below you mention a bug in parsing the data sent by the tpm. If I want to allow untrusted access through the kernel RM one should allow it. It should be based on file system permissions, not arbitrary checks in the driver. Also, the more code you write the more the argument that the parsing error could be in the Linux kernel. Maybe they just want to use the TPM driver to gain access to kernel mode and not the TPM. > > > > 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. 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. > > > When spaces are not used /dev/tpm0 is root only and has full > > unrestricted access. Wouldn't this be based on the changeable mode of /dev/tpm0? > > > > 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. > > > Jason > > > > Best regards, > -- > Javier Martinez Canillas > Software Engineer - Desktop Hardware Enablement Red Hat
On Mon, Nov 20, 2017 at 04:14:41PM +0000, Roberts, William C wrote: > That's policy, and shouldn't be hardcoded in the kernel. Let the DAC > permission model And LSMs sort that out. Of course this is what was done, there are two cdevs, one with full access to the TPM and one that runs through the RM. The distro/admin gets to choose how to set the up, as is typical. One of the use models for the RM cdev was to support unpriv access to that cdev, if the admin chooses to grant access to the /dev/ node. That work isn't quite complete, as it was envisioned to include kind of user space controlled command filter/restriction. Safe unpriv access is explicitly not a design goal of /dev/tpmX. Jason
On Mon, Nov 20, 2017 at 10:26:01AM +0100, Javier Martinez Canillas wrote: > 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. Somehow it became about both .. The kernel defaults the tpmrm to root only, so the distro can decide how to set it up. Some people are giving access to unpriv users. > 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. It seems like it, from what I can see, but only if the command is not supported.. You should double check with James of course. Jason
On Fri, Nov 17, 2017 at 11:07:24AM +0100, Javier Martinez Canillas wrote: > 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 <javierm@redhat.com> It is not a virtual TPM so I don't think that matters. It at least matters less than breaking the sandbox. /Jarkko
Hello Jarkko, On 11/21/2017 12:15 AM, Jarkko Sakkinen wrote: > On Fri, Nov 17, 2017 at 11:07:24AM +0100, Javier Martinez Canillas wrote: >> 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 <javierm@redhat.com> > > It is not a virtual TPM so I don't think that matters. It at least 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. > 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? > /Jarkko > [0]: https://www.trustedcomputinggroup.org/wp-content/uploads/TSS-TAB-and-Resource-Manager-00-91-PublicReview.pdf [1]: http://linuxplumbersconf.com/2017/ocw//system/presentations/4818/original/TPM2-kernel-evnet-app_tricca-sakkinen.pdf Best regards,
On 11/21/2017 10:07 AM, Javier Martinez Canillas wrote: > On 11/21/2017 12:15 AM, Jarkko Sakkinen wrote: > >> 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? > Sorry, this should had been: send a synthesized TPM response in the case that the command isn't supported. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
On Tue, Nov 21, 2017 at 10:07:34AM +0100, Javier Martinez Canillas wrote: > 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? > 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* > 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. > > 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? Nope. /Jarkko
On 11/21/2017 01:30 PM, Jarkko Sakkinen wrote: > On Tue, Nov 21, 2017 at 10:07:34AM +0100, Javier Martinez Canillas wrote: >> 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? > >> 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* > Fair enough. >> 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. > >>> 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? > > 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. > /Jarkko > Best regards,
> -----Original Message----- > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > Sent: Tuesday, November 21, 2017 4:30 AM > To: Javier Martinez Canillas <javierm@redhat.com> > Cc: linux-kernel@vger.kernel.org; Peter Huewe <peterhuewe@gmx.de>; Tricca, > Philip B <philip.b.tricca@intel.com>; Jason Gunthorpe > <jgunthorpe@obsidianresearch.com>; linux-integrity@vger.kernel.org; Roberts, > William C <william.c.roberts@intel.com> > Subject: Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation > fails > > On Tue, Nov 21, 2017 at 10:07:34AM +0100, Javier Martinez Canillas wrote: > > 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? > > > 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* > > > 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. The published parts are, granted many things are changing. > > > > 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? > > 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 > > /Jarkko
Apologies for the slow response. I didn't get switched over from tpmdd-devel to linux-integrity till just now. > On 11/21/2017 01:30 PM, Jarkko Sakkinen wrote: >> On Tue, Nov 21, 2017 at 10:07:34AM +0100, Javier Martinez Canillas >> wrote: >>> 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? >> >>> 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* >> > > 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. >>> 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. >> >>>> 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? >> >> 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. 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
Hello Philip, On 11/22/2017 06:16 PM, flihp wrote: > Apologies for the slow response. I didn't get switched over from > tpmdd-devel to linux-integrity till just now. > No worries, thanks a lot for your feedback. >> On 11/21/2017 01:30 PM, Jarkko Sakkinen wrote: >>> On Tue, Nov 21, 2017 at 10:07:34AM +0100, Javier Martinez Canillas >>> wrote: >>>> 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? >>> >>>> 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* >>> >> >> 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). > That was my interpretation as well and what I was arguing about. I'm glad to know that you also think the same. > 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. > >>>> 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. >>> >>>>> 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? >>> >>> 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. > > 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. > Agreed, as explained in my last email, the possible ways to fix in user-space would be workarounds for the kernel RM not being consistent and not following the TPM specification. Can you please comment on the RFCv2 patch I shared that sends a TPM response with the appropriate response code as suggested by Jason? I'm convinced that is the correct approach to handle this case. > Philip > Best regards,
On Wed, Nov 22, 2017 at 09:16:25AM -0800, flihp wrote: > 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 :) Jason
On Tue, Nov 21, 2017 at 08:29:07PM +0000, Roberts, William C wrote: > > TPM specification is not a formal specification AFAIK. > > The published parts are, granted many things are changing. Yes, how it defines the protocol, you are correct. It does not have a formal definition of RM behavior or at least I haven't found it. > > > 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. > > 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. /Jarkko
On Tue, Nov 21, 2017 at 01:49:23PM +0100, Javier Martinez Canillas wrote: > 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. Thanks for anyway putting that patch out. I'll investigate it with time and reconsider my position. Philip, can you share your view. Adding also James. /Jarkko
On Wed, Nov 22, 2017 at 09:16:25AM -0800, flihp wrote: > 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. 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
On Wed, Nov 22, 2017 at 08:25:29PM +0100, Javier Martinez Canillas wrote: > 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
On 11/26/2017 03:18 PM, Jarkko Sakkinen wrote: > On Wed, Nov 22, 2017 at 09:16:25AM -0800, flihp wrote: >> 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. > > 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? > My current version wasn't setting the error level bits. I'll send a new version that does this. I'll also drop the RFC prefix and propose it as a formal patch since it seems we are getting closer to an agreement and also add James as cc. > /Jarkko > Best regards,
On 11/26/2017 03:21 PM, Jarkko Sakkinen wrote: > On Wed, Nov 22, 2017 at 08:25:29PM +0100, Javier Martinez Canillas wrote: >> 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. > No worries, Philip did a much better work than I did at explaining the issue. In fact, at the beginning I also thought that was an user-space problem until he explained to me that the problem was in the kernel. > 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... > And I really appreciate. It's much better to go back and forth on patches than having an unstable interface that causes regressions between kernel releases. I've posted a v2 that addressed Philip's comments. Hopefully this should be in a good shape now. > /Jarkko > Best regards,
On 11/17/2017 5:07 AM, Javier Martinez Canillas wrote: > 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?. At a basic level, I agree with Javier. If the device driver cannot send the command to the TPM (e.g., if the size of the transmit buffer doesn't match commandSize), then the driver should return an error. Beyond those, IMHO the driver should pass through well formed commands and return the TPM response to the user level applications. Some reasons: - It's what the application developer expects. - The behavior should be (as much as possible) the same, whether one is using /dev/tpmrm0, /dev/tpm0, or the TPM simulator. - The application developer can use the TPM spec (or set a breakpoint in the simulator) to debug. -EINVAL requires the application developer to dig into kernel code. It also seems easier for the device driver, and so potentially will require less maintenance.
On 11/17/2017 11:57 AM, Jason Gunthorpe wrote: > On Fri, Nov 17, 2017 at 11:07:24AM +0100, Javier Martinez Canillas wrote: > >> 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. I think there is a difference between: - The kernel could __not__ accept the request, and - The kernel could accept the request, but there's code in the kernel to reject it. My preference is for the kernel to send the command when possible, and let the TPM decide whether it can be executed. Otherwise, the kernel device driver has to be updated every time the TPM implements something new that the kernel previously thought was an error.
On 11/17/2017 1:17 PM, Jason Gunthorpe wrote: > On Fri, Nov 17, 2017 at 07:10:09PM +0100, Javier Martinez Canillas wrote: > >> 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? To me, the simplest solution, and the easiest for the application developer to debug, is to pass through any command possible.
On 11/22/2017 3:13 PM, Jason Gunthorpe wrote: > On Wed, Nov 22, 2017 at 09:16:25AM -0800, flihp wrote: > >> 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. Second, if "handle" means "pass EINVAL back up to the application, which will not have any idea what it means", that's easy. But sending the application an error code or message that would permit them to debug is much harder. To me, it's like the difference between "file not found" and "disk error".
On Fri, Dec 08, 2017 at 03:03:34PM -0500, Ken Goldman wrote: > 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. We have to, it is required for securing unpriv access. > 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? The TPM arch already split the commands in a way where you don't need to look at params in most cases. I think we might, or should, look at params in some of the 'get cap' cases ? Jason
On 11/26/2017 9:06 AM, Jarkko Sakkinen wrote: > > 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?
On Fri, Dec 08, 2017 at 03:16:53PM -0500, Ken Goldman wrote: > 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. There are lots of failure cases that have nothing to do with the TPM, and I really don't want to see complex code mapping traditional POSIX errors into TPM packets in the kernel. If you need that, do it in userspace?? Jason
Hello Ken, On 12/08/2017 09:20 PM, Ken Goldman wrote: > On 11/26/2017 9:06 AM, Jarkko Sakkinen wrote: >> >> 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.) > At the end it was rather trivial. I posted that patch and got merged today: http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/40ee8cc47c95de22a34c10561f2c00e3c665a29f?hp=7b4edc34bc7542068fa530c0d38babdac95a5e46 > 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. Best regards,
On Fri, Dec 08, 2017 at 03:20:02PM -0500, Ken Goldman wrote: > On 11/26/2017 9:06 AM, Jarkko Sakkinen wrote: > > > > 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? Breaks the sandbox. /Jarkko
Ken, Javier, Here's my counterpart to you :-) Sorry for the delay. I'm quite busy upstreaming SGX ATM. > 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. 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
Hello Jarkko, On 12/17/2017 05:47 PM, Jarkko Sakkinen wrote: > Ken, Javier, > > Here's my counterpart to you :-) Sorry for the delay. I'm quite busy > upstreaming SGX ATM. > No worries, thanks for your feedback. >> 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. > > I would say kernel should keep the amount of policy minimal. Your > statement is a "textbook definition" what kernel should do. Sandbox Yes, sorry for attempting to simplify. We certainly have a lot of places in the kernel where policies are defined. > 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. > My point was that the patch bypassed all the TPM spaces code paths so it should not break the in-kernel RM state. But you are right that due lack of a TPMA_CC for the unknown command, we have no way to know what will be the side effects of sending the command to the TPM so the in-kernel RM and TPM states can get out of sync. It's actually a very good point and something that I didn't think about before. > 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): > My understanding is that Ken was referring to returning -EINVAL instead of a proper response with a TPM_RC_COMMAND_CODE code. But that got fixed with the patch you merged so I don't think he will have a problem with that solution. > https://trustedcomputinggroup.org/wp-content/uploads/TCG-TSS-2.0-Overview-and-Common-Structures-Specification-Version-0.90-Revision-02.pdf > > /Jarkko > Best regards,
On 12/17/2017 11:47 AM, Jarkko Sakkinen wrote: > 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): I think (?) my comment was around the suggestion that the SAPI TSS could map -EINVAL to TPM_RC_COMMAND_CODE. I.e, that all the user space TSS'es (not different RMs) would have to follow this tread and fix their implementations. I wrote: > 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.
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1d6729be4cd6..86527e9a27cc 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -390,9 +390,14 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, u32 count, ordinal; unsigned long stop; bool need_locality; + bool cmd_validated; - if (!tpm_validate_command(chip, space, buf, bufsiz)) - return -EINVAL; + /* + * If command validation fails, sent it to the TPM anyways so it can + * report a proper error to user-space. Just don't do any TPM space + * management in this case. + */ + cmd_validated = tpm_validate_command(chip, space, buf, bufsiz); if (bufsiz > TPM_BUFSIZE) bufsiz = TPM_BUFSIZE; @@ -424,9 +429,11 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, chip->locality = rc; } - rc = tpm2_prepare_space(chip, space, ordinal, buf); - if (rc) - goto out; + if (cmd_validated) { + rc = tpm2_prepare_space(chip, space, ordinal, buf); + if (rc) + goto out; + } rc = chip->ops->send(chip, (u8 *) buf, count); if (rc < 0) { @@ -481,7 +488,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space, goto out; } - rc = tpm2_commit_space(chip, space, ordinal, buf, &len); + if (cmd_validated) + rc = tpm2_commit_space(chip, space, ordinal, buf, &len); out: if (need_locality && chip->ops->relinquish_locality) {
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 <javierm@redhat.com> --- Hello, 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 Best regards, Javier drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)