linux-kernel - Re: [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20201204125821.GW5487@ziepe.ca>
Date:   Fri, 4 Dec 2020 08:58:21 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Sergey Temerkhanov <s.temerkhanov@...il.com>
Cc:     Peter Huewe <peterhuewe@....de>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jerry Snitselaar <jsnitsel@...hat.com>,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races

On Fri, Dec 04, 2020 at 01:18:05PM +0300, Sergey Temerkhanov wrote:
> Avoid race condition at shutdown by shutting downn the TPM 2.0
> devices synchronously. This eliminates the condition when the
> shutdown sequence sets chip->ops to NULL leading to the following:
> 
> [ 1586.593561][ T8669] tpm2_del_space+0x28/0x73
> [ 1586.598718][ T8669] tpmrm_release+0x27/0x33wq
> [ 1586.603774][ T8669] __fput+0x109/0x1d
> [ 1586.608380][ T8669] task_work_run+0x7c/0x90
> [ 1586.613414][ T8669] prepare_exit_to_usermode+0xb8/0x128
> [ 1586.619522][ T8669] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1586.626068][ T8669] RIP: 0033:0x4cb4bb
> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@...il.com>
>  drivers/char/tpm/tpm-chip.c  | 27 ---------------------------
>  drivers/char/tpm/tpm-dev.c   | 11 ++++++-----
>  drivers/char/tpm/tpmrm-dev.c |  7 +++++++
>  include/linux/tpm.h          |  2 +-
>  4 files changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 1838039b0333..ede7f4790c5e 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -282,32 +282,6 @@ static void tpm_devs_release(struct device *dev)
>  	put_device(&chip->dev);
>  }
>  
> -/**
> - * tpm_class_shutdown() - prepare the TPM device for loss of power.
> - * @dev: device to which the chip is associated.
> - *
> - * Issues a TPM2_Shutdown command prior to loss of power, as required by the
> - * TPM 2.0 spec. Then, calls bus- and device- specific shutdown code.
> - *
> - * Return: always 0 (i.e. success)
> - */
> -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 (!tpm_chip_start(chip)) {
> -			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> -			tpm_chip_stop(chip);
> -		}
> -	}
> -	chip->ops = NULL;
> -	up_write(&chip->ops_sem);
> -
> -	return 0;
> -}

This does more than just call tpm2_shutdown, it exists to prevent use
after free situations because the chip point can exist in other parts
of the system beyond the lifetime of the driver.

You can't call into the driver at all past shutdown, so moving
tpm2_shutdown here:

> @@ -39,6 +41,11 @@ static int tpmrm_release(struct inode *inode, struct file *file)
>  
>  	tpm_common_release(file, fpriv);
>  	tpm2_del_space(fpriv->chip, &priv->space);
> +
> +	if (!atomic_dec_return(&fpriv->chip->is_open)) {
> +		tpm2_shutdown(fpriv->chip, TPM2_SU_CLEAR);
> +		tpm_chip_stop(fpriv->chip);
> +	}

Is just wrong.

Your bug is because tpmrm_release is not following the lifetime rules
for chip, it probably needs to do a get on the pointer to be able to
access the ops.

Jason