linux-kernel - Re: [PATCH v2] tpm_tis: Add missing tpm_request/relinquish

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: <464454f440df67d3470e67ff0386bbc306d07dac.camel@kernel.org>
Date:   Sat, 30 Jan 2021 22:40:40 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Lukasz Majczak <lma@...ihalf.com>,
        Guenter Roeck <linux@...ck-us.net>,
        James Bottomley <James.Bottomley@...senpartnership.com>
Cc:     Tj <ml.linux@...oe.vision>, Dirk Gouders <dirk@...ders.net>,
        Peter Huewe <peterhuewe@....de>,
        Jason Gunthorpe <jgg@...pe.ca>,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org,
        Radoslaw Biernacki <rad@...ihalf.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Alex Levin <levinale@...gle.com>
Subject: Re: [PATCH v2] tpm_tis: Add missing tpm_request/relinquish_locality
 calls

On Thu, 2021-01-28 at 14:07 +0100, Lukasz Majczak wrote:
> There is a missing call to tpm_request_locality before the call to
> the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current
> approach might work for tpm2, it fails for tpm1.x - in that case
> call to tpm_get_timeouts() or tpm_tis_probe_irq_single()
> without locality fails and in turn causes tpm_tis_core_init() to fail.
> Tested on Samsung Chromebook Pro (Caroline).
> 
> Signed-off-by: Lukasz Majczak <lma@...ihalf.com>

Is it possible that you test against linux-next and see if any
problems still arise? I've applied the locality fixes from James.

> ---
> Jarkko, James, Guenter
> 
> I’m aware about the other thread, but it seems to be dead for a few months.
> Here is the small patch as fixing this specific issue
> would allow us to unblock the ChromeOs development. 
> We want to upstream all of our patches,
> so the ChromeOs will not diverge even more,
> so I'm hoping this could be applied, if you see it neat enough.

The usual approach is that you construct a series picking the pre-existing
fixes and on top of that create your own, if any required.

> Best regards,
> Lukasz

/Jarkko

> 
> v1 -> v2:
>  - fixed typos
>  - as there is no need to enable clock, switched to
>    use only tpm_request/relinquish_locality calls
>  - narrowed down boundaries of tpm_request/relinquish_locality calls
>  
>  drivers/char/tpm/tpm-chip.c      |  4 ++--
>  drivers/char/tpm/tpm-interface.c | 11 +++++++++--
>  drivers/char/tpm/tpm.h           |  2 ++
>  drivers/char/tpm/tpm_tis_core.c  | 12 ++++++++++--
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ddaeceb7e109..5351963a4b19 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -32,7 +32,7 @@ struct class *tpm_class;
>  struct class *tpmrm_class;
>  dev_t tpm_devt;
>  
> -static int tpm_request_locality(struct tpm_chip *chip)
> +int tpm_request_locality(struct tpm_chip *chip)
>  {
>         int rc;
>  
> @@ -47,7 +47,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
>         return 0;
>  }
>  
> -static void tpm_relinquish_locality(struct tpm_chip *chip)
> +void tpm_relinquish_locality(struct tpm_chip *chip)
>  {
>         int rc;
>  
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1621ce818705..69309b2bea6a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -243,8 +243,15 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  
>         if (chip->flags & TPM_CHIP_FLAG_TPM2)
>                 return tpm2_get_timeouts(chip);
> -       else
> -               return tpm1_get_timeouts(chip);
> +       else {
> +               ssize_t ret = tpm_request_locality(chip);
> +
> +               if (ret)
> +                       return ret;
> +               ret = tpm1_get_timeouts(chip);
> +               tpm_relinquish_locality(chip);
> +               return ret;
> +       }
>  }
>  EXPORT_SYMBOL_GPL(tpm_get_timeouts);
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 947d1db0a5cc..8c13008437dd 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -193,6 +193,8 @@ static inline void tpm_msleep(unsigned int delay_msec)
>  
>  int tpm_chip_start(struct tpm_chip *chip);
>  void tpm_chip_stop(struct tpm_chip *chip);
> +int tpm_request_locality(struct tpm_chip *chip);
> +void tpm_relinquish_locality(struct tpm_chip *chip);
>  struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
>  __must_check int tpm_try_get_ops(struct tpm_chip *chip);
>  void tpm_put_ops(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 92c51c6cfd1b..0ae675e8cf2f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -754,9 +754,17 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  
>         if (chip->flags & TPM_CHIP_FLAG_TPM2)
>                 return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
> -       else
> -               return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc,
> +       else {
> +               ssize_t ret = tpm_request_locality(chip);
> +
> +               if (ret)
> +                       return ret;
> +               ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc,
>                                   0);
> +               tpm_relinquish_locality(chip);
> +               return ret;
> +       }
> +
>  }
>  
>  /* Register the IRQ and issue a command that will cause an interrupt. If an