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;
Message-ID: <1594408113.4494.16.camel@HansenPartnership.com>
Subject: Re: [PATCH] tpm: avoid accessing cleared ops during shutdown
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Andrey Pronin <apronin@chromium.org>, peterhuewe@gmx.de,
        jarkko.sakkinen@linux.intel.com
Cc:     jgg@ziepe.ca, linux-integrity@vger.kernel.org,
        linux-kernel@vger.kernel.org, groeck@chromium.org
Date:   Fri, 10 Jul 2020 12:08:33 -0700
In-Reply-To: <20200710002209.6757-1-apronin@chromium.org>
References: <20200710002209.6757-1-apronin@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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