linux-kernel - Re: [PATCH] tpm: avoid accessing cleared ops during shutdown

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1594408113.4494.16.camel@HansenPartnership.com>
Date:   Fri, 10 Jul 2020 12:08:33 -0700
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Andrey Pronin <apronin@...omium.org>, peterhuewe@....de,
        jarkko.sakkinen@...ux.intel.com
Cc:     jgg@...pe.ca, linux-integrity@...r.kernel.org,
        linux-kernel@...r.kernel.org, groeck@...omium.org
Subject: Re: [PATCH] tpm: avoid accessing cleared ops during shutdown

On Thu, 2020-07-09 at 17:22 -0700, Andrey Pronin wrote:
> This patch prevents NULL dereferencing when using chip->ops while
> sending TPM2_Shutdown command if both tpm_class_shutdown handler and
> tpm_del_char_device are called during system shutdown.
> 
> Both these handlers set chip->ops to NULL but don't check if it's
> already NULL when they are called before using it.
> 
> This issue was revealed in Chrome OS after a recent set of changes
> to the unregister order for spi controllers, such as:
>   b4c6230bb0ba spi: Fix controller unregister order
>   f40913d2dca1 spi: pxa2xx: Fix controller unregister order
> and similar for other controllers.
> 
> Signed-off-by: Andrey Pronin <apronin@...omium.org>
> ---
>  drivers/char/tpm/tpm-chip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
> chip.c
> index 8c77e88012e9..a410ca40a3c5 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -296,7 +296,7 @@ static int tpm_class_shutdown(struct device *dev)
>  	struct tpm_chip *chip = container_of(dev, struct tpm_chip,
> dev);
>  
>  	down_write(&chip->ops_sem);
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +	if (chip->ops && (chip->flags & TPM_CHIP_FLAG_TPM2)) {
>  		if (!tpm_chip_start(chip)) {
>  			tpm2_shutdown(chip, TPM2_SU_CLEAR);
>  			tpm_chip_stop(chip);
> @@ -479,7 +479,7 @@ static void tpm_del_char_device(struct tpm_chip
> *chip)
>  
>  	/* Make the driver uncallable. */
>  	down_write(&chip->ops_sem);
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +	if (chip->ops && (chip->flags & TPM_CHIP_FLAG_TPM2)) {
>  		if (!tpm_chip_start(chip)) {
>  			tpm2_shutdown(chip, TPM2_SU_CLEAR);
>  			tpm_chip_stop(chip);

I really don't think this is the right fix.  The problem is that these
two functions are trying to open code tpm_try_get_ops/tpm_put_ops (only
really for the tpm2 shutdown) because they want to NULL out the ops
before final mutex unlock.  The problem with the current open coding is
it doesn't shut down the clock if required (not really a problem for
shutdown, but might cause issues for simple rmmod).  I think this is
because no-one noticed the open coding when get/put was updated.

This code should all be abstracted into a single function and shared
with tpm_try_get_ops/tpm_put_ops so we can't have this happen in
future.  Possibly there should be a chip shutdown function which is
only active for TPM2 which does the correct try_get/shutdown/put
operation and then a separate simple get mutex, null ops, put mutex one
that's guaranteed to be called last.

James