More changes related to TPM RNG handling

[PATCH 1/3] tpm: Add a missing check for TPM_CHIP_FLAG_HWRNG_DISABLED

Posted by Mario Limonciello 2 years, 6 months ago

If the TPM is opted out of hwrng the error handling for
tpm_chip_register() needs to know this so it doesn't try to clean
up an uninitialized chip->hwrng.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/char/tpm/tpm-chip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e904aae9771be..8f61b784810d6 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -629,7 +629,8 @@ int tpm_chip_register(struct tpm_chip *chip)
 	return 0;
 
 out_hwrng:
-	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip))
+	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip) &&
+	    !(chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED))
 		hwrng_unregister(&chip->hwrng);
 out_ppi:
 	tpm_bios_log_teardown(chip);
-- 
2.34.1

Re: [PATCH 1/3] tpm: Add a missing check for TPM_CHIP_FLAG_HWRNG_DISABLED

Posted by Jarkko Sakkinen 2 years, 6 months ago

On Thu Aug 3, 2023 at 4:50 AM EEST, Mario Limonciello wrote:
> If the TPM is opted out of hwrng the error handling for
> tpm_chip_register() needs to know this so it doesn't try to clean
> up an uninitialized chip->hwrng.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/char/tpm/tpm-chip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e904aae9771be..8f61b784810d6 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -629,7 +629,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>  	return 0;
>  
>  out_hwrng:
> -	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip))
> +	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip) &&
> +	    !(chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED))
>  		hwrng_unregister(&chip->hwrng);
>  out_ppi:
>  	tpm_bios_log_teardown(chip);
> -- 
> 2.34.1

Please add a fixes tag.

BR, Jarkko

Re: [PATCH 1/3] tpm: Add a missing check for TPM_CHIP_FLAG_HWRNG_DISABLED

Posted by Mario Limonciello 2 years, 6 months ago

On 8/3/23 03:59, Jarkko Sakkinen wrote:
> On Thu Aug 3, 2023 at 4:50 AM EEST, Mario Limonciello wrote:
>> If the TPM is opted out of hwrng the error handling for
>> tpm_chip_register() needs to know this so it doesn't try to clean
>> up an uninitialized chip->hwrng.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index e904aae9771be..8f61b784810d6 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -629,7 +629,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>>   	return 0;
>>   
>>   out_hwrng:
>> -	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip))
>> +	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip) &&
>> +	    !(chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED))
>>   		hwrng_unregister(&chip->hwrng);
>>   out_ppi:
>>   	tpm_bios_log_teardown(chip);
>> -- 
>> 2.34.1
> 
> Please add a fixes tag.
> 
> BR, Jarkko

I didn't add a fixes tag because you hadn't sent a PR for the other one 
yet so I wasn't sure the hash would be stable.  Also I thought it might 
just make sense to squash into it.

If the hash is now stable, could you just just commit and add that tag 
with it yourself?  Or do you want me to re-send as a v2 with that?

Re: [PATCH 1/3] tpm: Add a missing check for TPM_CHIP_FLAG_HWRNG_DISABLED

Posted by Jason A. Donenfeld 2 years, 6 months ago

On Thu, Aug 03, 2023 at 06:35:36AM -0500, Mario Limonciello wrote:
> On 8/3/23 03:59, Jarkko Sakkinen wrote:
> > On Thu Aug 3, 2023 at 4:50 AM EEST, Mario Limonciello wrote:
> >> If the TPM is opted out of hwrng the error handling for
> >> tpm_chip_register() needs to know this so it doesn't try to clean
> >> up an uninitialized chip->hwrng.
> >>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >>   drivers/char/tpm/tpm-chip.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> >> index e904aae9771be..8f61b784810d6 100644
> >> --- a/drivers/char/tpm/tpm-chip.c
> >> +++ b/drivers/char/tpm/tpm-chip.c
> >> @@ -629,7 +629,8 @@ int tpm_chip_register(struct tpm_chip *chip)
> >>   	return 0;
> >>   
> >>   out_hwrng:
> >> -	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip))
> >> +	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip) &&
> >> +	    !(chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED))
> >>   		hwrng_unregister(&chip->hwrng);
> >>   out_ppi:
> >>   	tpm_bios_log_teardown(chip);
> >> -- 
> >> 2.34.1
> > 
> > Please add a fixes tag.
> > 
> > BR, Jarkko
> 
> I didn't add a fixes tag because you hadn't sent a PR for the other one 
> yet so I wasn't sure the hash would be stable.  Also I thought it might 
> just make sense to squash into it.
> 
> If the hash is now stable, could you just just commit and add that tag 
> with it yourself?  Or do you want me to re-send as a v2 with that?

What about just sending a v3 of the patch that this patch fixes? The
stable@/fixes tags in that are wrong/incomplete so Jarkko's tree will
need to be fixed before pushing to Linus anyway.

Jason

Re: [PATCH 1/3] tpm: Add a missing check for TPM_CHIP_FLAG_HWRNG_DISABLED

Posted by Jarkko Sakkinen 2 years, 6 months ago

On Thu Aug 3, 2023 at 4:50 PM EEST, Jason A. Donenfeld wrote:
> On Thu, Aug 03, 2023 at 06:35:36AM -0500, Mario Limonciello wrote:
> > On 8/3/23 03:59, Jarkko Sakkinen wrote:
> > > On Thu Aug 3, 2023 at 4:50 AM EEST, Mario Limonciello wrote:
> > >> If the TPM is opted out of hwrng the error handling for
> > >> tpm_chip_register() needs to know this so it doesn't try to clean
> > >> up an uninitialized chip->hwrng.
> > >>
> > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > >> ---
> > >>   drivers/char/tpm/tpm-chip.c | 3 ++-
> > >>   1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > >> index e904aae9771be..8f61b784810d6 100644
> > >> --- a/drivers/char/tpm/tpm-chip.c
> > >> +++ b/drivers/char/tpm/tpm-chip.c
> > >> @@ -629,7 +629,8 @@ int tpm_chip_register(struct tpm_chip *chip)
> > >>   	return 0;
> > >>   
> > >>   out_hwrng:
> > >> -	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip))
> > >> +	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip) &&
> > >> +	    !(chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED))
> > >>   		hwrng_unregister(&chip->hwrng);
> > >>   out_ppi:
> > >>   	tpm_bios_log_teardown(chip);
> > >> -- 
> > >> 2.34.1
> > > 
> > > Please add a fixes tag.
> > > 
> > > BR, Jarkko
> > 
> > I didn't add a fixes tag because you hadn't sent a PR for the other one 
> > yet so I wasn't sure the hash would be stable.  Also I thought it might 
> > just make sense to squash into it.
> > 
> > If the hash is now stable, could you just just commit and add that tag 
> > with it yourself?  Or do you want me to re-send as a v2 with that?
>
> What about just sending a v3 of the patch that this patch fixes? The
> stable@/fixes tags in that are wrong/incomplete so Jarkko's tree will
> need to be fixed before pushing to Linus anyway.

Sounds reasonable. I can hold the PR to rc6 and send it on Monday.

BR, Jarkko