Re: [PATCH v4] tpm: Add Upgrade/Reduced mode support for TPM2 modules

[Jarkko Sakkinen]

On Fri, Aug 06, 2021 at 04:18:08PM +0200, Borys Movchan wrote:
> If something went wrong during the TPM firmware upgrade, like power
> failure or the firmware image file get corrupted, the TPM might end
> up in Upgrade or Failure mode upon the next start. The state is
> persistent between the TPM power cycle/restart.
> 
> According to TPM specification:
>  * If the TPM is in Upgrade mode, it will answer with TPM2_RC_UPGRADE
>    to all commands except Field Upgrade related ones.
>  * If the TPM is in Failure mode, it will allow performing TPM
>    initialization but will not provide any crypto operations.
>    Will happily respond to Field Upgrade calls.
> 
> Change the behavior of the tpm2_auto_startup(), so it detects the active
> running mode of the TPM.  It is easy to determine that TPM is in Upgrade
> mode by relying on the fact that tpm2_do_selftest() will return
> TPM2_RC_UPGRADE. In such a case, there is no point to finish the
> start-up procedure as the TPM will not accept any commands, except
> firmware upgrade related.
> 
> On the other hand, if the TPM is in Failure mode, it will successfully
> respond to both tpm2_do_selftest() and tpm2_startup() calls. Although,
> will fail to answer to tpm2_get_cc_attrs_tbl(). Use this fact to
> conclude that TPM is in Failure mode.
> 
> If the chip is in the Upgrade or Failure mode, the function returns -EIO
> error code.
> 
> The return value is checked in the tpm_chip_register() call to determine
> the state of the TPM. If the TPM is not in normal operation mode, set
> the `limited_mode` flag. If the flag is set then the TPM is not able to

Nit: do not use hyphens for limited mode. 'limited_mode' is fine. I'm
also fine with just limited_mode.

> provide any crypto functionality.  Correspondignly, the calls to
> tpm2_get_cc_attrs_tbl(), tpm_add_hwrng() and tpm_get_pcr_allocation()
> will fail. Use the flag to exclude them from the initialization
> sequence.

This is blacklisting. E.g. I'm not sure why all of the sysfs attributes
would still be exported. Some of them use TPM commands. That was just
one random example I came up with.

It's easy to come up other examples, like, why you provide still tpmrm0,
which is dependent on a TPM running normal mode?

This misses completely the rationale for ever acking this change: which
parts of the uapi are export and *why*.

Please whitelist the things that should still work. Even the obvious
ones like /dev/tpm0 (because of TPM_RC_UPGRADE).

This is clearly a faulty and incomplete patch in its current form.

/Jarkko