Re: [PATCH] tpm: Don't make vendor check required for probe

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 2:57 AM EEST, Limonciello, Mario wrote:
>
>
> On 8/18/2023 6:44 PM, Jarkko Sakkinen wrote:
> > On Fri Aug 18, 2023 at 10:38 PM UTC, Limonciello, Mario wrote:
> >>
> >>
> >> On 8/18/2023 5:07 PM, Jarkko Sakkinen wrote:
> >>> On Fri Aug 18, 2023 at 6:15 PM UTC, Mario Limonciello wrote:
> >>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for
> >>>> all AMD fTPMs") doesn't work properly on Intel fTPM.  The TPM doesn't reply
> >>>> at bootup and returns back the command code.
> >>>
> >>> Is this reproducible with any production hardware? You are stating it
> >>> as it was reproducible categorically with any Intel fTPM.
> >>>
> >>
> >> Yes, it's affecting production hardware too.
> >> Someone came to the kernel bugzilla and reported a regression on 6.4.11
> >> on a Lenovo Intel laptop as well.
> > 
> > Now the description says that cateogrically all Intel fTPM's fail.
>
> According to Todd this change caused 5 *different* reference Intel 
> systems all to fail.  I know they're not production hardware, but still..

Kind of sane requirement is to scope on known systems that exists. We
are not fixing bugs for R&D platforms.

>
> > 
> > I asked for the laptop model in the bugzilla bug, which should be put to
> > the commit description later on (hopefully with a snippet of klog
> > transcript). This commit cannot be applied as it is at the moment, even
> > if it turned out to be a legit fix.
> > 
> >>>> As this isn't crucial for anything but AMD fTPM and AMD fTPM works, throw
> >>>> away the error code to let Intel fTPM continue to work.
> >>>>
> >>>> Cc: stable@xxxxxxxxxxxxxxx
> >>>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs")
> >>>
> >>> It does make sense not to exercise this outside of AMD CPus but since
> >>> there is no production hardware failing, it cannot be categorized as a
> >>> bug fix.
> >>
> >> See above (and also kernel bugzilla).
> >>
> >>>
> >>>> Reported-by: Todd Brandt <todd.e.brandt@xxxxxxxxx>
> >>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804
> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>>> ---
> >>>>    drivers/char/tpm/tpm_crb.c | 3 +--
> >>>>    1 file changed, 1 insertion(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> >>>> index 9eb1a18590123..b0e9931fe436c 100644
> >>>> --- a/drivers/char/tpm/tpm_crb.c
> >>>> +++ b/drivers/char/tpm/tpm_crb.c
> >>>> @@ -472,8 +472,7 @@ static int crb_check_flags(struct tpm_chip *chip)
> >>>>    	if (ret)
> >>>>    		return ret;
> >>>>    
> >>>> -	ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL);
> >>>> -	if (ret)
> >>>> +	if (tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL))
> >>>>    		goto release;
> >>>
> >>> It would be better not to exercise a potentially failing code path at
> >>> all. This initiates full transaction with the TPM.
> >>
> >> So why does a full transaction not work in this case?
> > 
> > It makes absolutely zero sense to send a message to a TPM just to know
> > that you are on AMD CPU, right?
> > 
> > E.g. you could check if boot_cpu_data.x86_vendor == X86_VENDOR_AMD right
> > in the beginning of this function.
> > 
>
> You couldn't replace the whole function with this check.
>
> If you apply it to everything on an AMD system you catch too much.  You 
> need to know it's specifically an AMD fTPM to avoid tripping the stutter 
> bug.  Pluton is fine, dTPMs are fine too.

dTPM's have nothing to be with tpm_crb. They use tpm_tis_core.

Is there a system with CPU recognized as AMD and *firmware* TPM from
some 3rd party vendor? If not, then you could also drop
tpm2_get_tpm_pt() altogether.


BR, Jarkko