linux-kernel - Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)

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: <eca48e99-3aad-d2fc-c073-aa5ff18c9b51@linux.ibm.com>
Date:   Tue, 21 Dec 2021 09:17:00 -0500
From:   Stefan Berger <stefanb@...ux.ibm.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>
Cc:     peterhuewe@....de, linux-integrity@...r.kernel.org, jgg@...pe.ca,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, pavrampu@...ibm.com,
        Korrapati.Likhitha@....com, gcwilson@...ibm.com
Subject: Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer
 (powerpc)


On 12/21/21 09:01, Stefan Berger wrote:
>
> On 12/21/21 03:47, Jarkko Sakkinen wrote:
>> On Sat, Dec 11, 2021 at 08:28:04PM -0500, Stefan Berger wrote:
>>> Fix the following crash on kexec by checking chip->ops for a NULL 
>>> pointer
>>> in tpm_chip_start() and returning an error code if this is the case.
>>>
>>> BUG: Kernel NULL pointer dereference on read at 0x00000060
>>> Faulting instruction address: 0xc00000000099a06c
>>> Oops: Kernel access of bad area, sig: 11 [#1]
>>> ...
>>> NIP [c00000000099a06c] tpm_chip_start+0x2c/0x140
>>>   LR [c00000000099a808] tpm_chip_unregister+0x108/0x170
>>> Call Trace:
>>> [c0000000188bfa00] [c000000002b03930] fw_devlink_strict+0x0/0x8 
>>> (unreliable)
>>> [c0000000188bfa30] [c00000000099a808] tpm_chip_unregister+0x108/0x170
>>> [c0000000188bfa70] [c0000000009a3874] tpm_ibmvtpm_remove+0x34/0x130
>>> [c0000000188bfae0] [c000000000110dbc] vio_bus_remove+0x5c/0xb0
>>> [c0000000188bfb20] [c0000000009bc154] device_shutdown+0x1d4/0x3a8
>>> [c0000000188bfbc0] [c000000000196e14] kernel_restart_prepare+0x54/0x70
>>>
>>> The referenced patch below introduced a function to shut down the 
>>> VIO bus.
>>> The bus shutdown now calls tpm_del_char_device (via 
>>> tpm_chip_unregister)
>>> after a call to tpm_class_shutdown, which already set chip->ops to 
>>> NULL.
>>> The crash occurrs when tpm_del_char_device calls tpm_chip_start with 
>>> the
>>> chip->ops NULL pointer.
>>>
>>> Fixes: 39d0099f9439 ("powerpc/pseries: Add shutdown() to vio_driver 
>>> and vio_bus")
>>> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
>>> ---
>>>   drivers/char/tpm/tpm-chip.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>> index ddaeceb7e109..cca1bde296ee 100644
>>> --- a/drivers/char/tpm/tpm-chip.c
>>> +++ b/drivers/char/tpm/tpm-chip.c
>>> @@ -101,6 +101,9 @@ int tpm_chip_start(struct tpm_chip *chip)
>>>   {
>>>       int ret;
>>>   +    if (!chip->ops)
>>> +        return -EINVAL;
>> This triggers to all drivers, not just tpm_ibmvtpm, i.e. the fix has
>> side-effects.
>
> What are those side-effects?
>
>
I am asking because if one entered tpm_chip_start() with chip->ops = 
NULL it would crash any system. So now the side-effect is that one can 
call this function without crashing the system but gets an -EINVAL back.

Another alternative that prevents these crashes is this change here 
including code deduplication:

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ddaeceb7e109..888d37293091 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);
@@ -473,15 +473,7 @@ static void tpm_del_char_device(struct tpm_chip *chip)
         mutex_unlock(&idr_lock);

         /* Make the driver uncallable. */
-       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);
+       tpm_class_shutdown(&chip->dev);
  }

  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)



     Stefan