Re: [PATCH 1/3] char: tpm: Protect tpm_pm_suspend with locks

From: "Jan Dąbroś" <jsd@semihalf.com>
To: Tim Van Patten <timvp@google.com>
Cc: linux-integrity@vger.kernel.org, jarkko@kernel.org,
	peterhuewe@gmx.de, jgg@ziepe.ca, gregkh@linuxfoundation.org,
	arnd@arndb.de, rrangel@chromium.org, apronin@google.com,
	mw@semihalf.com, upstream@semihalf.com
Subject: Re: [PATCH 1/3] char: tpm: Protect tpm_pm_suspend with locks
Date: Wed, 2 Nov 2022 22:28:36 +0100	[thread overview]
Message-ID: <CAOtMz3Od-r03EigmuEAG_q27bvifRS8ZV8YPOUHrayEW6YXO-Q@mail.gmail.com> (raw)
In-Reply-To: <CANkg5eyWqhReHJd7Bj5EEG5chz89M-PKCnak91qPRWZEzm3NRw@mail.gmail.com>

> > -       if (!tpm_chip_start(chip)) {
> > +       rc = tpm_try_get_ops(chip);
> > +       if (!rc) {
> >                 if (chip->flags & TPM_CHIP_FLAG_TPM2)
> >                         tpm2_shutdown(chip, TPM2_SU_STATE);
> >                 else
> >                         rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
>
> This if-else block is still interacting with the TPM even though
> you're not guaranteed to have the lock, which could lead to
> racy/inchorent results. Would it be better to just bail out entirely
> since we can't safely attempt any recovery at this point. If it's
> still worth attempting the shutdown command, it would at least be good
> to add a comment admitting that we have no choice but to communicate
> with the TPM without a lock.

If tpm_try_get_ops() returns 0 it means that we have a lock. And if we
don't have a lock, then we are not executing any TPM commands. Are you
referring to tpm_mutex or something different?

Best Regards,
Jan