Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 4 Dec 2020 08:58:21 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Sergey Temerkhanov <s.temerkhanov@gmail.com>
Cc:     Peter Huewe <peterhuewe@gmx.de>,
        Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jerry Snitselaar <jsnitsel@redhat.com>,
        linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races
Message-ID: <20201204125821.GW5487@ziepe.ca>
References: <20201204101805.27374-1-s.temerkhanov@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201204101805.27374-1-s.temerkhanov@gmail.com>
Precedence: bulk

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@gmail.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