linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Limonciello, Mario" <mario.limonciello@amd.com>
Cc: <linux-integrity@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	"Thorsten Leemhuis" <regressions@leemhuis.info>,
	<charles.d.prestopine@intel.com>, <rafael.j.wysocki@intel.com>,
	<len.brown@intel.com>, <stable@vger.kernel.org>,
	"Todd Brandt" <todd.e.brandt@intel.com>
Subject: Re: [PATCH] tpm: Don't make vendor check required for probe
Date: Tue, 22 Aug 2023 14:25:09 +0300	[thread overview]
Message-ID: <CUZ1BDCFZ7S8.26LGWT9U1C87H@suppilovahvero> (raw)
In-Reply-To: <719602ec-c9fc-4a72-9585-d50595cb6dca@amd.com>

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@vger.kernel.org
> >>>> 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@intel.com>
> >>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804
> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>>> ---
> >>>>    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

  parent reply	other threads:[~2023-08-22 11:25 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-18 18:15 [PATCH] tpm: Don't make vendor check required for probe Mario Limonciello
2023-08-18 22:07 ` Jarkko Sakkinen
2023-08-18 22:38   ` Limonciello, Mario
2023-08-18 23:44     ` Jarkko Sakkinen
2023-08-18 23:57       ` Limonciello, Mario
2023-08-19 13:35         ` Patrick Steinhardt
2023-08-22 11:25         ` Jarkko Sakkinen [this message]
2023-08-28 15:47 ` Dusty Mabe
2023-09-04 18:15   ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CUZ1BDCFZ7S8.26LGWT9U1C87H@suppilovahvero \
    --to=jarkko@kernel.org \
    --cc=charles.d.prestopine@intel.com \
    --cc=len.brown@intel.com \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mario.limonciello@amd.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=regressions@leemhuis.info \
    --cc=stable@vger.kernel.org \
    --cc=todd.e.brandt@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).