[RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[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.

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(-)

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) {
-- 
2.14.3

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Roberts, William C]

> -----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

RE: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Roberts, William C]

> -----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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Roberts, William C]

> -----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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

RE: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Roberts, William C]

> -----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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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

[snip]

>>>
>>> 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 <javierm@redhat.com>
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 <jgg@ziepe.ca>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

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(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ebe0a1d36d8c..b8d01897c0ba 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -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;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c1866cc02e30..40818fa59b05 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -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,
 };
-- 
2.14.3

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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

[snip]

>>>
>>> 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:

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[flihp]

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

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

On Wed, Nov 22, 2017 at 10:26:24AM +0100, Javier Martinez Canillas wrote:
> On 11/21/2017 09:29 PM, Roberts, William C wrote:
> 
> [snip]
> 
> >>>
> >>> 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:

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

> From 145b6891a68b32ae803a4b0abd3d35679ed6b2a1 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> 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 <jgg@ziepe.ca>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> ---
> 
> 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(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index ebe0a1d36d8c..b8d01897c0ba 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -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;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index c1866cc02e30..40818fa59b05 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -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,
>  };
> -- 
> 2.14.3

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

On 11/26/2017 03:12 PM, Jarkko Sakkinen wrote:
> On Wed, Nov 22, 2017 at 10:26:24AM +0100, Javier Martinez Canillas wrote:
>> On 11/21/2017 09:29 PM, Roberts, William C wrote:
>>
>> [snip]
>>
>>>>>
>>>>> 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:
> 
> 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.
>

Please don't worry. My rule of thumb when posting patches to LKML is to wait at
least 2 weeks before asking for the patch to the maintainer. So your answer was
much faster than I expected :)

Also if we decide that this should be fixed at the kernel-space, then it doesn't
block any tpm2-{tss,tools} release.
 
> I'll go through the patch within next few days.
>

Thanks a lot for your help.

> [1] https://lkml.org/lkml/2017/11/25/123
> 
> /Jarkko
> 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Ken Goldman]

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.

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Ken Goldman]

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.

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Ken Goldman]

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.

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Ken Goldman]

On 11/22/2017 4:26 AM, Javier Martinez Canillas wrote:
> 
> I still do believe that both interfaces (/dev/tpm and /dev/tpmrm) should be
> consistent if possible though. 

Agreed.  TPM code is hard enough to debug without inconsistencies 
between tpm, tpmrm, and the simulator.

> 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.

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.

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Ken Goldman]

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".

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Ken Goldman]

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?

Re: FW: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jason Gunthorpe]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Jarkko Sakkinen]

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

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [RFC PATCH] tpm: don't return -EINVAL if TPM command validation fails

[Ken Goldman]

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.