| Message ID | 600d6778-f155-ea51-64dc-e0041c6943d3@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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,
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.
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, };