| Message ID | 20171129110846.31892-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 29, 2017 at 12:08:46PM +0100, Javier Martinez Canillas wrote: > +#define TPM2_RC_LAYER_SHIFT 16 > +#define TPM2_RESMGRTPM_RC_LAYER (11 << TPM2_RC_LAYER_SHIFT) I got this spec from Philip [1]. Couple of remarks: * What is the difference between TSS2_RESMGR_RC_LAYER and TSS2_RESMGR_TPM_RC_LAYER? * Should the driver code use TSS2 or TPM2 prefix? [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG-TSS-2.0-Overview-and-Common-Structures-Specification-Version-0.90-Revision-02.pdf /Jarkko
Hello Jarkko, On 11/29/2017 06:57 PM, Jarkko Sakkinen wrote: > On Wed, Nov 29, 2017 at 12:08:46PM +0100, Javier Martinez Canillas > wrote: >> +#define TPM2_RC_LAYER_SHIFT 16 +#define TPM2_RESMGRTPM_RC_LAYER >> (11 << TPM2_RC_LAYER_SHIFT) > > I got this spec from Philip [1]. > > Couple of remarks: > > * What is the difference between TSS2_RESMGR_RC_LAYER and > TSS2_RESMGR_TPM_RC_LAYER? The difference is the type of error returned in each case. TSS2_RESMGR_RC_LAYER means that's an error internal to the TAB/RM and so the response code is one of the TSS2_BASE_RC_* error values. But TSS2_RESMGR_TPM_RC_LAYER means that the resource manager is taking over some TPM functionality (i.e: validation) and so the response code is a TSS2_RC_* error value, liket is the case for this patch (TPM_RC_COMMAND_CODE). > * Should the driver code use TSS2 or TPM2 prefix? > That's a very good question. I used TPM2 as prefix instead of TSS2 to keep it consistent with the rest of the driver, but probably TSS2 should be used instead so people can search more easy the constant in the specification doc. > [1] > https://trustedcomputinggroup.org/wp-content/uploads/TCG-TSS-2.0-Overview-and-Common-Structures-Specification-Version-0.90-Revision-02.pdf > > /Jarkko > Best regards,
On 11/29/2017 09:57 AM, Jarkko Sakkinen wrote: > On Wed, Nov 29, 2017 at 12:08:46PM +0100, Javier Martinez Canillas wrote: >> +#define TPM2_RC_LAYER_SHIFT 16 >> +#define TPM2_RESMGRTPM_RC_LAYER (11 << TPM2_RC_LAYER_SHIFT) > > I got this spec from Philip [1]. As part of this I've been doing a pass back over the current draft spec fixing up all of the places where the we were using 'ERROR_LEVEL', 'RESPONSE_LEVEL' etc interchangeably. Everything will be 'RC_LAYER' in the next published draft. Apologies if this caused any confusion here but it looks like Javier was able to work around this regardless Next time around the spec should be more clear. Philip > Couple of remarks: > > * What is the difference between TSS2_RESMGR_RC_LAYER and > TSS2_RESMGR_TPM_RC_LAYER? > * Should the driver code use TSS2 or TPM2 prefix? > > [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG-TSS-2.0-Overview-and-Common-Structures-Specification-Version-0.90-Revision-02.pdf > > /Jarkko >
On 11/29/2017 10:24 AM, Javier Martinez Canillas wrote: > Hello Jarkko, > > On 11/29/2017 06:57 PM, Jarkko Sakkinen wrote: >> On Wed, Nov 29, 2017 at 12:08:46PM +0100, Javier Martinez Canillas >> wrote: >>> +#define TPM2_RC_LAYER_SHIFT 16 +#define TPM2_RESMGRTPM_RC_LAYER >>> (11 << TPM2_RC_LAYER_SHIFT) >> >> I got this spec from Philip [1]. >> >> Couple of remarks: >> >> * What is the difference between TSS2_RESMGR_RC_LAYER and >> TSS2_RESMGR_TPM_RC_LAYER? > > The difference is the type of error returned in each case. TSS2_RESMGR_RC_LAYER > means that's an error internal to the TAB/RM and so the response code is one of > the TSS2_BASE_RC_* error values. > > But TSS2_RESMGR_TPM_RC_LAYER means that the resource manager is taking over some > TPM functionality (i.e: validation) and so the response code is a TSS2_RC_* error > value, liket is the case for this patch (TPM_RC_COMMAND_CODE). This distinction predates my participation in the spec. Personally I don't think users will really care so long as it's evident which 'layer' produced the error. Using the TSS2_RESMGR_TPM_RC_LAYER is the right thing to do though according to the spec. >> * Should the driver code use TSS2 or TPM2 prefix? >> > > That's a very good question. I used TPM2 as prefix instead of TSS2 to keep it > consistent with the rest of the driver, but probably TSS2 should be used instead > so people can search more easy the constant in the specification doc. +1 Philip >> [1] >> https://trustedcomputinggroup.org/wp-content/uploads/TCG-TSS-2.0-Overview-and-Common-Structures-Specification-Version-0.90-Revision-02.pdf >> >> /Jarkko >> > > Best regards, >
On Wed, Nov 29, 2017 at 07:24:48PM +0100, Javier Martinez Canillas wrote: > Hello Jarkko, > > On 11/29/2017 06:57 PM, Jarkko Sakkinen wrote: > > On Wed, Nov 29, 2017 at 12:08:46PM +0100, Javier Martinez Canillas > > wrote: > >> +#define TPM2_RC_LAYER_SHIFT 16 +#define TPM2_RESMGRTPM_RC_LAYER > >> (11 << TPM2_RC_LAYER_SHIFT) > > > > I got this spec from Philip [1]. > > > > Couple of remarks: > > > > * What is the difference between TSS2_RESMGR_RC_LAYER and > > TSS2_RESMGR_TPM_RC_LAYER? > > The difference is the type of error returned in each case. TSS2_RESMGR_RC_LAYER > means that's an error internal to the TAB/RM and so the response code is one of > the TSS2_BASE_RC_* error values. > > But TSS2_RESMGR_TPM_RC_LAYER means that the resource manager is taking over some > TPM functionality (i.e: validation) and so the response code is a TSS2_RC_* error > value, liket is the case for this patch (TPM_RC_COMMAND_CODE). > > > * Should the driver code use TSS2 or TPM2 prefix? > > > > That's a very good question. I used TPM2 as prefix instead of TSS2 to keep it > consistent with the rest of the driver, but probably TSS2 should be used instead > so people can search more easy the constant in the specification doc. OK, I'll change the prefix. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> I'll postpone testing to next week as I try to get v7 of the SGX patch set done during this week. I'll add test case or two for this to my smoke test suite (contributions are of course welcome): https://github.com/jsakkine-intel/tpm2-scripts /Jarkko
On Wed, Nov 29, 2017 at 06:13:51PM -0800, Philip Tricca wrote: > On 11/29/2017 10:24 AM, Javier Martinez Canillas wrote: > > Hello Jarkko, > > > > On 11/29/2017 06:57 PM, Jarkko Sakkinen wrote: > >> On Wed, Nov 29, 2017 at 12:08:46PM +0100, Javier Martinez Canillas > >> wrote: > >>> +#define TPM2_RC_LAYER_SHIFT 16 +#define TPM2_RESMGRTPM_RC_LAYER > >>> (11 << TPM2_RC_LAYER_SHIFT) > >> > >> I got this spec from Philip [1]. > >> > >> Couple of remarks: > >> > >> * What is the difference between TSS2_RESMGR_RC_LAYER and > >> TSS2_RESMGR_TPM_RC_LAYER? > > > > The difference is the type of error returned in each case. TSS2_RESMGR_RC_LAYER > > means that's an error internal to the TAB/RM and so the response code is one of > > the TSS2_BASE_RC_* error values. > > > > But TSS2_RESMGR_TPM_RC_LAYER means that the resource manager is taking over some > > TPM functionality (i.e: validation) and so the response code is a TSS2_RC_* error > > value, liket is the case for this patch (TPM_RC_COMMAND_CODE). > > This distinction predates my participation in the spec. Personally I > don't think users will really care so long as it's evident which 'layer' > produced the error. Using the TSS2_RESMGR_TPM_RC_LAYER is the right > thing to do though according to the spec. > > >> * Should the driver code use TSS2 or TPM2 prefix? > >> > > > > That's a very good question. I used TPM2 as prefix instead of TSS2 to keep it > > consistent with the rest of the driver, but probably TSS2 should be used instead > > so people can search more easy the constant in the specification doc. > > +1 Please response with Reviewed/Tested-by if these changes work for you. /Jarkko
On 11/30/2017 05:38 PM, Jarkko Sakkinen wrote: > On Wed, Nov 29, 2017 at 07:24:48PM +0100, Javier Martinez Canillas wrote: >> Hello Jarkko, >> >> On 11/29/2017 06:57 PM, Jarkko Sakkinen wrote: >>> On Wed, Nov 29, 2017 at 12:08:46PM +0100, Javier Martinez Canillas >>> wrote: >>>> +#define TPM2_RC_LAYER_SHIFT 16 +#define TPM2_RESMGRTPM_RC_LAYER >>>> (11 << TPM2_RC_LAYER_SHIFT) >>> >>> I got this spec from Philip [1]. >>> >>> Couple of remarks: >>> >>> * What is the difference between TSS2_RESMGR_RC_LAYER and >>> TSS2_RESMGR_TPM_RC_LAYER? >> >> The difference is the type of error returned in each case. TSS2_RESMGR_RC_LAYER >> means that's an error internal to the TAB/RM and so the response code is one of >> the TSS2_BASE_RC_* error values. >> >> But TSS2_RESMGR_TPM_RC_LAYER means that the resource manager is taking over some >> TPM functionality (i.e: validation) and so the response code is a TSS2_RC_* error >> value, liket is the case for this patch (TPM_RC_COMMAND_CODE). >> >>> * Should the driver code use TSS2 or TPM2 prefix? >>> >> >> That's a very good question. I used TPM2 as prefix instead of TSS2 to keep it >> consistent with the rest of the driver, but probably TSS2 should be used instead >> so people can search more easy the constant in the specification doc. > > OK, I'll change the prefix. > No need for you to do that since I posted a v3 today that changed the prefix: https://lkml.org/lkml/2017/11/30/56 I also added Philip's Reviewed-by tag, since he said I could do as a response to my v1: https://lkml.org/lkml/2017/11/27/1297 To save you following the link: > Thanks for incorporating my feedback into your patch. Feel free to add > the appropriate tag to the commit message to document my review if it's > appropriate. > > Philip > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Thanks! > I'll postpone testing to next week as I try to get v7 of the SGX patch > set done during this week. > Sure, no worries. Keep it mind though that in order to test you need to find a TPM2 that doesn't implement a command or inject a command lookup failure. I faced this issue on an Intel PTT fTPM2.0 (Thinkpad X1 Carbon 4th gen) with the tpm2_encryptdecrypt tool from the tpm2-tools project. The tool first try to use TPM2_EncryptDecrypt2 (which isn't supported by the fTPM2) and then it fallbacks to TPM2_EncryptDecrypt. > I'll add test case or two for this to my smoke test suite (contributions > are of course welcome): > > https://github.com/jsakkine-intel/tpm2-scripts > I can do that. Probably what you want is send_cmd() to also unpack the layer RC bits and ProtocolError() to print from what layer the RC is coming from? > /Jarkko > Best regards,
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1d6729be4cd6..ac83ad149ec7 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 | + TPM2_RESMGRTPM_RC_LAYER); + return bufsiz; + } if (bufsiz > TPM_BUFSIZE) bufsiz = TPM_BUFSIZE; diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 2d5466a72e40..55833b7fcc1a 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -93,12 +93,17 @@ enum tpm2_structures { TPM2_ST_SESSIONS = 0x8002, }; +/* Indicates from what level of the software stack the error comes from */ +#define TPM2_RC_LAYER_SHIFT 16 +#define TPM2_RESMGRTPM_RC_LAYER (11 << TPM2_RC_LAYER_SHIFT) + enum tpm2_return_codes { TPM2_RC_SUCCESS = 0x0000, TPM2_RC_HASH = 0x0083, /* RC_FMT1 */ 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, };
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 that response as an 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 not implemented and the TPM responds with an error. Instead of returning an -EINVAL errno code when the tpm_validate_command() function fails, synthesize a TPM command response so user-space can get a TPM_RC_COMMAND_CODE as expected when a chip doesn't implement the command. The TPM only sets 12 of the 32 bits in the TPM_RC response, so the TSS and TAB specifications define that higher layers in the stack should use some of the unused 20 bits to specify from which level of the stack the error is coming from. Since the TPM_RC_COMMAND_CODE response code is sent by the kernel resource manager, set the error level to the TAB/RM layer so user-space is aware of this. Suggested-by: Jason Gunthorpe <jgg@ziepe.ca> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- Changes since v1: - Remove the unused macros for the driver and resmgr RC layers (suggested by Philip Tricca). - Use naming conventions from the latest version of the TSS spec (suggested by Philip Tricca). Changes since RFCv2: - Set the error level to the TAB/RM layer so user-space is aware that the error is not coming from the TPM (suggested by Philip Tricca and Jarkko Sakkinen). Changes since RFCv1: - Don't pass not validated commands to the TPM, instead return a synthesized response with the correct TPM return code (suggested by Jason Gunthorpe). 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 | 28 ++++++++++++++++++++-------- drivers/char/tpm/tpm.h | 5 +++++ 2 files changed, 25 insertions(+), 8 deletions(-)