Messages in this thread Patch in this message | | | Date | Tue, 21 Dec 2021 09:17:00 -0500 | Subject | Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc) | From | Stefan Berger <> |
| |
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@linux.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
| |