Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Stefan Berger <stefanb@linux.ibm.com>]

On 7/4/24 02:41, Jarkko Sakkinen wrote:
> On Thu Jul 4, 2024 at 4:56 AM EEST, Stefan Berger wrote:
>>
>>
>> On 7/3/24 14:24, Jarkko Sakkinen wrote:
>>> Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can
>>
>> Doesn't tpm_chip_register() need to be called by all drivers? This
>> function then calls tpm_chip_bootstrap().
>>
>>> cause a null derefence in tpm_buf_hmac_session*().  Thus, address
>>> !chip->auth in tpm_buf_hmac_session*() and remove the fallback
>>> implementation for !TCG_TPM2_HMAC.
>>>
>>> Cc: stable@vger.kernel.org # v6.9+
>>> Reported-by: Stefan Berger <stefanb@linux.ibm.com>
>>> Closes: https://lore.kernel.org/linux-integrity/20240617193408.1234365-1-stefanb@linux.ibm.com/
>>> Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>
>> I applied this series now but it doesn't solve the reported problem. The
> 
> It fixes the issues of which symptoms was shown by your transcript:
> 
> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error 0x01C4
> [    2.987140] ima: Error Communicating to TPM chip, result: -14
> 
> Your original thread identified zero problems, so thus your claim here
> is plain untrue.

The original thread here

https://lore.kernel.org/linux-integrity/656b319fc58683e399323b880722434467cf20f2.camel@kernel.org/T/#t

identified the fact that tpm2_session_init() was missing for the ibmvtpm 
driver. It is a non-zero problem for the respective platforms where this 
driver is being used. The patched fixed the reported issue.

> 
> Before the null derefence is fixed all other patches related are
> blocked, including ibm_tpmvtpm patches, because it would be insane
> to accept them when there is known memory corruption bug, which
> this patch set fixes.
> 
> What is so difficult to understand in this?
> 
>> error message is gone but the feature can still be enabled
>> (CONFIG_TCG_TPM2_HMAC=y) but is unlikely actually doing what it is
>> promising to do with this config option. So you either still have to
>> apply my patch, James's patch, or your intended "depends on
>> !TCG_IBMVTPM" patch.
> 
> Well this somewhat misleading imho...
> 
> None of the previous patches, including your, do nothing to fix the null
> derefence bug and that is the *only* bug we care about ATM. With these
> fixes drivers that do not call tpm_chip_bootstrap() will be fully
> working still but without encryption.
> 

Now that you fixed it in v4 are you going to accept my original patch 
with the Fixes tag since we will (likely) have an enabled feature in 
6.10 that is not actually working when the ibmvtpm driver is being used?

Original patch:

https://lore.kernel.org/linux-integrity/656b319fc58683e399323b880722434467cf20f2.camel@kernel.org/T/#t

> There's five drivers which would require update for that:
> 
> drivers/char/tpm/tpm_ftpm_tee.c:        pvt_data->chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_i2c_nuvoton.c:             chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_ibmvtpm.c:         chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_tis_i2c_cr50.c:    chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_vtpm_proxy.c:              proxy_dev->chip->flags |= TPM_CHIP_FLAG_TPM2;

I do no think that this is true and its only tpm_ibmvtpm.c that need the 
call to tpm2_session_init. All drivers that use TPM_OPS_AUTO_STARTUP 
will run tpm_chip_register -> tpm_chip_bootstrap -> tpm_auto_startup -> 
tpm2_auto_startup -> tpm2_sessions_init

$ grep AUTO_START *.c
tpm_crb.c:      .flags = TPM_OPS_AUTO_STARTUP,
tpm_ftpm_tee.c: .flags = TPM_OPS_AUTO_STARTUP,
tpm_i2c_atmel.c:        .flags = TPM_OPS_AUTO_STARTUP,
tpm_i2c_infineon.c:     .flags = TPM_OPS_AUTO_STARTUP,
tpm_i2c_nuvoton.c:      .flags = TPM_OPS_AUTO_STARTUP,
tpm-interface.c:        if (!(chip->ops->flags & TPM_OPS_AUTO_STARTUP))
tpm_tis_core.c: .flags = TPM_OPS_AUTO_STARTUP,
tpm_tis_i2c_cr50.c:     .flags = TPM_OPS_AUTO_STARTUP,
tpm_vtpm_proxy.c:       .flags = TPM_OPS_AUTO_STARTUP,

All the above drivers are also calling tpm_chip_register.

tpm_atmel.c:    rc = tpm_chip_register(chip);
tpm-chip.c: * tpm_chip_register() - create a character device for the 
TPM chip
tpm-chip.c:int tpm_chip_register(struct tpm_chip *chip)
tpm-chip.c:EXPORT_SYMBOL_GPL(tpm_chip_register);
tpm-chip.c: * cleans up all the resources reserved by tpm_chip_register().
tpm_crb.c:      rc = tpm_chip_register(chip);
tpm_ftpm_tee.c: rc = tpm_chip_register(pvt_data->chip);
tpm_ftpm_tee.c:         dev_err(dev, "%s: tpm_chip_register failed with 
rc=%d\n",
tpm_i2c_atmel.c:        return tpm_chip_register(chip);
tpm_i2c_infineon.c:     return tpm_chip_register(chip);
tpm_i2c_nuvoton.c:      return tpm_chip_register(chip);
tpm_ibmvtpm.c:  return tpm_chip_register(chip);
tpm_infineon.c:         rc = tpm_chip_register(chip);
tpm_nsc.c:      rc = tpm_chip_register(chip);
tpm_tis_core.c: rc = tpm_chip_register(chip);
tpm_tis_i2c_cr50.c:     return tpm_chip_register(chip);
tpm_vtpm_proxy.c:       rc = tpm_chip_register(proxy_dev->chip);
xen-tpmfront.c: return tpm_chip_register(priv->chip)


   Stefan

> 
> 
> BR, Jarkko