[v3] tpm: Enable hwrng only for Pluton on AMD CPUs

  The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for
all AMD fTPMs") doesn't work properly on a number of Intel fTPMs.  On the
reported systems the TPM doesn't reply at bootup and returns back the
command code. This makes the TPM fail probe.

Since only Microsoft Pluton is the only known combination of AMD CPU and
fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin
aware of this, print also info message to the klog.

Cc: stable@vger.kernel.org
Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs")
Reported-by: Todd Brandt <todd.e.brandt@intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v3:
* Forgot to amend config flags.
v2:
* CONFIG_X86
* Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>"
* Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>"
---
 drivers/char/tpm/tpm_crb.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)
  

Dear Jarkko,


Thank you for your patch.


Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen:
> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for
> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs.  On the
> reported systems the TPM doesn't reply at bootup and returns back the
> command code. This makes the TPM fail probe.
> 
> Since only Microsoft Pluton is the only known combination of AMD CPU and
> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin
> aware of this, print also info message to the klog.
> 
> Cc: stable@vger.kernel.org
> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs")
> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Mario’s patch also had the three reporters below listed:

Reported-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Ronan Pigott <ronan@rjp.ie>
Reported-by: Raymond Jay Golo <rjgolo@gmail.com>

> ---
> v3:
> * Forgot to amend config flags.
> v2:
> * CONFIG_X86
> * Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>"
> * Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>"
> ---
>   drivers/char/tpm/tpm_crb.c | 33 ++++++++-------------------------
>   1 file changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 65ff4d2fbe8d..ea085b14ab7c 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -463,28 +463,6 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
>   	return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE;
>   }
>   
> -static int crb_check_flags(struct tpm_chip *chip)
> -{
> -	u32 val;
> -	int ret;
> -
> -	ret = crb_request_locality(chip, 0);
> -	if (ret)
> -		return ret;
> -
> -	ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL);
> -	if (ret)
> -		goto release;
> -
> -	if (val == 0x414D4400U /* AMD */)
> -		chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED;
> -
> -release:
> -	crb_relinquish_locality(chip, 0);
> -
> -	return ret;
> -}
> -
>   static const struct tpm_class_ops tpm_crb = {
>   	.flags = TPM_OPS_AUTO_STARTUP,
>   	.status = crb_status,
> @@ -827,9 +805,14 @@ static int crb_acpi_add(struct acpi_device *device)
>   	if (rc)
>   		goto out;
>   
> -	rc = crb_check_flags(chip);
> -	if (rc)
> -		goto out;
> +#ifdef CONFIG_X86
> +	/* A quirk for https://www.amd.com/en/support/kb/faq/pa-410 */
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> +	    priv->sm != ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON) {
> +		dev_info(dev, "Disabling hwrng\n");

A more elaborate log message would be helpful for the user. Maybe:

Disabling hwrng in AMD's fTPM to avoid stutter (AMD article PA 410)

> +		chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED;
> +	}
> +#endif /* CONFIG_X86 */
>   
>   	rc = tpm_chip_register(chip);
>   


Kind regards,

Paul
  

On Wed, 2023-08-23 at 21:24 +0200, Paul Menzel wrote:
> [Cc: +Andy, +Joe]
> 
> 
> Dear Jarkko, dear Andy, dear Joe,
> 
> 
> Am 23.08.23 um 19:40 schrieb Jarkko Sakkinen:
> > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote:
> 
> > > Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen:
> > > > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for
> > > > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs.  On the
> > > > reported systems the TPM doesn't reply at bootup and returns back the
> > > > command code. This makes the TPM fail probe.
> > > > 
> > > > Since only Microsoft Pluton is the only known combination of AMD CPU and
> > > > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin
> > > > aware of this, print also info message to the klog.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs")
> > > > Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > 
> > > Mario’s patch also had the three reporters below listed:
> > > 
> > > Reported-by: Patrick Steinhardt <ps@pks.im>
> > > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > 
> > The problem here is that checkpatch throws three warnings:
> > 
> > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > #19:
> > Reported-by: Patrick Steinhardt <ps@pks.im>
> > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > 
> > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > #20:
> > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > 
> > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > #21:
> > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Since bugzilla is not part of the documented process afaik, I used this
> > field as the guideline:
> > 
> > Reported:	2023-08-17 20:59 UTC by Todd Brandt
> > 
> > How otherwise I should interpret kernel bugzilla?
> 
> How is the proper process to add more than one reporter (so they are 
> noted and also added to CC), so that checkpatch.pl does not complain?
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> > In any case new version is still needed as the commit message must
> > contain a mention of "Lenovo Legion Y540" as the stimulus for doing
> > this code change in the first place.
> > 
> > BR, Jarkko

Well, if it's really desired to allow multiple consecutive reported-by:
lines, maybe:
---
 scripts/checkpatch.pl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 528f619520eb9..5b5c11ad04087 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3179,6 +3179,8 @@ sub process {
 				if (!defined $lines[$linenr]) {
 					WARN("BAD_REPORTED_BY_LINK",
 					     "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . "\n");
+				} elsif ($rawlines[$linenr] =~ /^\s*reported(?:|-and-tested)-by:/i) {
+					;
 				} elsif ($rawlines[$linenr] !~ /^closes:\s*/i) {
 					WARN("BAD_REPORTED_BY_LINK",
 					     "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
  

On Wed Aug 23, 2023 at 10:24 PM EEST, Paul Menzel wrote:
> [Cc: +Andy, +Joe]
>
>
> Dear Jarkko, dear Andy, dear Joe,
>
>
> Am 23.08.23 um 19:40 schrieb Jarkko Sakkinen:
> > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote:
>
> >> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen:
> >>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for
> >>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs.  On the
> >>> reported systems the TPM doesn't reply at bootup and returns back the
> >>> command code. This makes the TPM fail probe.
> >>>
> >>> Since only Microsoft Pluton is the only known combination of AMD CPU and
> >>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin
> >>> aware of this, print also info message to the klog.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs")
> >>> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804
> >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> >>
> >> Mario’s patch also had the three reporters below listed:
> >>
> >> Reported-by: Patrick Steinhardt <ps@pks.im>
> >> Reported-by: Ronan Pigott <ronan@rjp.ie>
> >> Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > 
> > The problem here is that checkpatch throws three warnings:
> > 
> > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > #19:
> > Reported-by: Patrick Steinhardt <ps@pks.im>
> > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > 
> > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > #20:
> > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > 
> > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > #21:
> > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Since bugzilla is not part of the documented process afaik, I used this
> > field as the guideline:
> > 
> > Reported:	2023-08-17 20:59 UTC by Todd Brandt
> > 
> > How otherwise I should interpret kernel bugzilla?
>
> How is the proper process to add more than one reporter (so they are 
> noted and also added to CC), so that checkpatch.pl does not complain?

I have no idea. I actually tried all sorts of combinations with no luck.

Since it exists and is out there, the process documentation should
really bring up some clarity to the kernel bugzilla.

BR, Jarkko
  

On Thu Aug 24, 2023 at 7:43 AM EEST, Joe Perches wrote:
> On Wed, 2023-08-23 at 21:24 +0200, Paul Menzel wrote:
> > [Cc: +Andy, +Joe]
> > 
> > 
> > Dear Jarkko, dear Andy, dear Joe,
> > 
> > 
> > Am 23.08.23 um 19:40 schrieb Jarkko Sakkinen:
> > > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote:
> > 
> > > > Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen:
> > > > > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for
> > > > > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs.  On the
> > > > > reported systems the TPM doesn't reply at bootup and returns back the
> > > > > command code. This makes the TPM fail probe.
> > > > > 
> > > > > Since only Microsoft Pluton is the only known combination of AMD CPU and
> > > > > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin
> > > > > aware of this, print also info message to the klog.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs")
> > > > > Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804
> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > 
> > > > Mario’s patch also had the three reporters below listed:
> > > > 
> > > > Reported-by: Patrick Steinhardt <ps@pks.im>
> > > > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > > 
> > > The problem here is that checkpatch throws three warnings:
> > > 
> > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > > #19:
> > > Reported-by: Patrick Steinhardt <ps@pks.im>
> > > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > > 
> > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > > #20:
> > > Reported-by: Ronan Pigott <ronan@rjp.ie>
> > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > > 
> > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> > > #21:
> > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com>
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > 
> > > Since bugzilla is not part of the documented process afaik, I used this
> > > field as the guideline:
> > > 
> > > Reported:	2023-08-17 20:59 UTC by Todd Brandt
> > > 
> > > How otherwise I should interpret kernel bugzilla?
> > 
> > How is the proper process to add more than one reporter (so they are 
> > noted and also added to CC), so that checkpatch.pl does not complain?
> > 
> > 
> > Kind regards,
> > 
> > Paul
> > 
> > 
> > > In any case new version is still needed as the commit message must
> > > contain a mention of "Lenovo Legion Y540" as the stimulus for doing
> > > this code change in the first place.
> > > 
> > > BR, Jarkko
>
> Well, if it's really desired to allow multiple consecutive reported-by:
> lines, maybe:
> ---
>  scripts/checkpatch.pl | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 528f619520eb9..5b5c11ad04087 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3179,6 +3179,8 @@ sub process {
>  				if (!defined $lines[$linenr]) {
>  					WARN("BAD_REPORTED_BY_LINK",
>  					     "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . "\n");
> +				} elsif ($rawlines[$linenr] =~ /^\s*reported(?:|-and-tested)-by:/i) {
> +					;
>  				} elsif ($rawlines[$linenr] !~ /^closes:\s*/i) {
>  					WARN("BAD_REPORTED_BY_LINK",
>  					     "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n");

Kind of opposing this because:

1. Bugzilla has a reporter field.
2. The request is now, if I understood this correctly, to add
   reported-by field to all people who have left a comment.
3. There is a field for the reporter, which points out to a single
   person. Why all the possible commenters and not the creator
   of the report?

BR, Jarkko
  

On Tue Sep 5, 2023 at 3:01 PM EEST, Thorsten Leemhuis wrote:
> On 05.09.23 00:32, Jarkko Sakkinen wrote:
> > On Fri Sep 1, 2023 at 11:49 AM EEST, Thorsten Leemhuis wrote:
> >> On 29.08.23 10:38, Linux regression tracking (Thorsten Leemhuis) wrote:
> >>> On 28.08.23 02:35, Mario Limonciello wrote:
> >>>> On 8/27/2023 13:12, Jarkko Sakkinen wrote:
> >>>>> On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote:
> >>>>>> On 8/23/2023 12:40, Jarkko Sakkinen wrote:
> >>>>>>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote:
> >>>>>>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen:
> >>>>>>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable
> >>>>>>>>> RNG for
> >>>>>>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. 
> >>>>>>>>> On the
> >>>>>>>>> reported systems the TPM doesn't reply at bootup and returns back the
> >>>>>>>>> command code. This makes the TPM fail probe.
> > [...]
> >> Hmmm. Quite a bit progress to fix the issue was made in the first week
> >> after Todd's report; Jarkko apparently even applied the earlier patch
> >> from Mario to his master branch:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b1a62d41bdc1d15b0641759717e8c3651f0a810c
> >> But since then (aka in the past week) there was not much progress.
>
> Jarkko, many thx for picking this up and submitting it to Linus, much
> appreciated. Sorry again for prodding things, but I felt I had to. Hope
> you didn't mind too much.
>
> > Could it be possible to extend the actual kernel documentation
> > to give at least some guidelines how a maintainer should deal
> > with the bugzilla?
>
> I guess it's best if that is done by somebody that cares about bugzilla
> (I don't fall into that group[1]) and knows the official status.
>
> But FWIW, I wonder what you actually want to see documented. From
> https://lore.kernel.org/all/CVAC8VQPD3PK.1CBS5QTWDSS2C@suppilovahvero/
> it sounds like you had trouble with Link:/Closes: tag and Reported-by.
> From what I can see I don't think bugzilla.kernel.org needs special
> documentation in that area:
>
>  * just use Link:/Closes: to reports to public reports that might be
> helpful later in case somebody wants to look at the backstory of a
> commit, wherever those reports may be (lore, bugzilla.kernel.org,
> https://gitlab.freedesktop.org/drm/intel/-/issues,
> https://github.com/thesofproject/linux/issues, ...)
>
>  * use Reported-by: to give credit to anyone that deserves it, as it is
> a nice way to say thx while motivate people to help again in the future.
> That usually will include the initial reporter, but might also include
> people that replied to a report from somebody else and helped
> perceptible with debugging or fixing.

I *kind of* agree with this but checkpatch.pl disagrees with this :-/

And it is an actual issue that bugzilla is hosted in kernel.org domain,
and at the same time it is undocumented process.

AFAIK anything that is not part of the process can be ignored in the
process so *theoretically* anything put to kernel bugzilla ca be
ignored. This is how it is with e.g. patchwork - some people use it,
some people don't.

Personally I think bugzilla, being user approachable system, should
be better defined but *theoretically*, at least by the process, it
can be fully ignored.

This is where the main source of confusion inherits from, when you
put your maintainer hat on.

> Ciao, Thorsten
>
> [1] I only sometimes help people that report regressions to
> bugzilla.kernel.org that otherwise would likely would fall through the
> cracks (among others because many reports are never forwarded to the
> proper developers otherwise).

BR, Jarkko