linux-kernel - Re: [PATCH v17 1/6] tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception

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: <cdeec193f1c1f5294581f349253e8863e64c9f5b.camel@kernel.org>
Date:   Tue, 26 Oct 2021 02:41:12 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     amirmizi6@...il.com, Eyal.Cohen@...oton.com,
        oshrialkoby85@...il.com, alexander.steffen@...ineon.com,
        robh+dt@...nel.org, mark.rutland@....com, peterhuewe@....de,
        jgg@...pe.ca, arnd@...db.de, gregkh@...uxfoundation.org,
        benoit.houyere@...com, eajames@...ux.ibm.com, joel@....id.au
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-integrity@...r.kernel.org, oshri.alkoby@...oton.com,
        tmaimon77@...il.com, gcwilson@...ibm.com, kgoldman@...ibm.com,
        Dan.Morav@...oton.com, oren.tanami@...oton.com,
        shmulik.hager@...oton.com, amir.mizinski@...oton.com
Subject: Re: [PATCH v17 1/6] tpm_tis: Fix expected bit handling and send all
 bytes in one shot without last byte in exception

On Sun, 2021-10-24 at 19:48 +0300, amirmizi6@...il.com wrote:
> From: Amir Mizinski <amirmizi6@...il.com>
> 
> Currently, the driver polls the TPM_STS.stsValid field until TRUE; then it
> reads TPM_STS register again to verify only that TPM_STS.expect field is
> FALSE (i.e., it ignores TPM_STS.stsValid).
> Since TPM_STS.stsValid represents the TPM_STS.expect validity, a check of
> only one of these fields is wrong. Fix this condition so that both fields
> are checked in the same TPM_STS register read.
> 
> Modify the signature of wait_for_tpm_stat(), adding an additional
> "result" parameter to its call.
> wait_for_tpm_stat() is now polling the TPM_STS with a mask and waits
> for the value in result. This modification adds the ability to check if
> certain TPM_STS bits have been cleared.
> For example, use the new parameter to check in status that TPM_STS_VALID
> is set and also that TPM_STS_EXPECT is zeroed. This prevents a racy
> check.
> 
> Fixes: 27084efee0c3 ("tpm: driver for next generation TPM chips")

Where does this failure occur in practice? If nowhere, this can be
dropped, and the patch can be considered as a feature. Defining it
as a fix makes only sense, if it needs to be backported to stable
kernels. This requires something to be actually broken.

The commit message does not contain a real bug report. It is just
referring to the specification, which is not a workload.

> Suggested-by: Benoit Houyere <benoit.houyere@...com>
> Signed-off-by: Amir Mizinski <amirmizi6@...il.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 59 ++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 69579ef..98de2fd 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -44,9 +44,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
>         return false;
>  }
>  
> -static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> -               unsigned long timeout, wait_queue_head_t *queue,
> -               bool check_cancel)
> +static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, u8 result,
> +                                unsigned long timeout,
> +                                wait_queue_head_t *queue, bool check_cancel)

I would consider renaming this as, given that you are changing the
signature anyway:

tpm_tis_wait_for_stat()

This would be more consistent with the other naming, and make e.g.
grepping kernel tree easier.

How did you end up to the name "result"? I have hard time deriving
from that name the actual semantics. E.g. "expected" would already
a way more sane name.

/Jarkko