| Message ID | 20211212012804.1555661-1-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: Fix kexec crash due to access to ops NULL pointer (powerpc) | expand |
> On 12-Dec-2021, at 6:58 AM, Stefan Berger <stefanb@linux.ibm.com> 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> > --- With the patch applied, kexec with vTPM works as expected. Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com> Thanks -Sachin
On 12/11/21 5:28 PM, 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. It was unclear to me at first, but IIUC the problem is the ibmvtpm device receives two shutdown calls, the first is a class shutdown call for TPM devices, followed by a bus shutdown call for VIO devices. It appears that the class shutdown routines are meant to be a pre-shutdown routine as they are defined as class->shutdown_pre(), and it is clearly allowed to call class->shutdown_pre() followed by one of but not both of the following bus->shutdown() or driver->shutdown(). Even tpm_class_shutdown() mentions in the function comment that bus/device shutdown to follow. > > Fixes: 39d0099f9439 ("powerpc/pseries: Add shutdown() to vio_driver and vio_bus") This patch left implementing each vio driver shutdown routine as an exercise for the respective maintainers, and instead just big hammers anything that doesn't have a shutdown routine by calling the driver->remove(). If tpm_class_shutdown() quiecses ibmvtpm enough implementing a no-op ibmvtpm->shutdown() with a comment saying so suffices. However, the generic TPM code is still introducing a bug that an attempt to call tpm_chip_unregister() after tpm_class_shutdown() will crash as mentioned above. > 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; > + I wonder if a better fix would to have tpm_del_char_device() check for valid chip->ops and call tpm_class_shutdown() when the ops are still valid. Calling tpm_class_shutdown() allows for some code deduplication in tpm_del_char_device(). -Tyrel > tpm_clk_enable(chip); > > if (chip->locality == -1) { >
On 12/20/21 19:39, Tyrel Datwyler wrote: > On 12/11/21 5:28 PM, 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. > It was unclear to me at first, but IIUC the problem is the ibmvtpm device > receives two shutdown calls, the first is a class shutdown call for TPM devices, > followed by a bus shutdown call for VIO devices. > > It appears that the class shutdown routines are meant to be a pre-shutdown > routine as they are defined as class->shutdown_pre(), and it is clearly allowed > to call class->shutdown_pre() followed by one of but not both of the following > bus->shutdown() or driver->shutdown(). Even tpm_class_shutdown() mentions in the > function comment that bus/device shutdown to follow. I suppose you are referring to this here: /** * tpm_class_shutdown() - prepare the TPM device for loss of power. * @dev: device to which the chip is associated. * * Issues a TPM2_Shutdown command prior to loss of power, as required by the * TPM 2.0 spec. Then, calls bus- and device- specific shutdown code. * * Return: always 0 (i.e. success) */ I think this comment still refers to the ancient code here: https://elixir.bootlin.com/linux/v4.4.295/source/drivers/char/tpm/tpm-chip.c#L161 if (dev->bus && dev->bus->shutdown) dev->bus->shutdown(dev); else if (dev->driver && dev->driver->shutdown) dev->driver->shutdown(dev); >> Fixes: 39d0099f9439 ("powerpc/pseries: Add shutdown() to vio_driver and vio_bus") > This patch left implementing each vio driver shutdown routine as an exercise for > the respective maintainers, and instead just big hammers anything that doesn't > have a shutdown routine by calling the driver->remove(). > > If tpm_class_shutdown() quiecses ibmvtpm enough implementing a no-op > ibmvtpm->shutdown() with a comment saying so suffices. > > However, the generic TPM code is still introducing a bug that an attempt to call > tpm_chip_unregister() after tpm_class_shutdown() will crash as mentioned above. An alternative solution may be this here: diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e109..4cb908349b31 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -473,15 +473,8 @@ 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); + if (chip->ops) + tpm_class_shutdown(&chip->dev); } static void tpm_del_legacy_sysfs(struct tpm_chip *chip) I could post this as v2 ?! Let me know... Stefan >> 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; >> + > I wonder if a better fix would to have tpm_del_char_device() check for valid > chip->ops and call tpm_class_shutdown() when the ops are still valid. Calling > tpm_class_shutdown() allows for some code deduplication in tpm_del_char_device(). > > -Tyrel > >> tpm_clk_enable(chip); >> >> if (chip->locality == -1) { >>
On Mon, Dec 20, 2021 at 08:05:58PM -0500, Stefan Berger wrote: > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index ddaeceb7e109..4cb908349b31 100644 > +++ b/drivers/char/tpm/tpm-chip.c > @@ -473,15 +473,8 @@ 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); > + if (chip->ops) ops cannot be read without locking Jason
On 12/20/21 20:13, Jason Gunthorpe wrote: > On Mon, Dec 20, 2021 at 08:05:58PM -0500, Stefan Berger wrote: > >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >> index ddaeceb7e109..4cb908349b31 100644 >> +++ b/drivers/char/tpm/tpm-chip.c >> @@ -473,15 +473,8 @@ 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); >> + if (chip->ops) > ops cannot be read without locking This here could be an alternative: diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e109..7772b475ebc0 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -474,7 +474,7 @@ static void tpm_del_char_device(struct tpm_chip *chip) /* Make the driver uncallable. */ 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); Stefan > > Jason
On 12/20/21 5:05 PM, Stefan Berger wrote: > > On 12/20/21 19:39, Tyrel Datwyler wrote: >> On 12/11/21 5:28 PM, 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. >> It was unclear to me at first, but IIUC the problem is the ibmvtpm device >> receives two shutdown calls, the first is a class shutdown call for TPM devices, >> followed by a bus shutdown call for VIO devices. >> >> It appears that the class shutdown routines are meant to be a pre-shutdown >> routine as they are defined as class->shutdown_pre(), and it is clearly allowed >> to call class->shutdown_pre() followed by one of but not both of the following >> bus->shutdown() or driver->shutdown(). Even tpm_class_shutdown() mentions in the >> function comment that bus/device shutdown to follow. > > I suppose you are referring to this here: > > /** > * tpm_class_shutdown() - prepare the TPM device for loss of power. > * @dev: device to which the chip is associated. > * > * Issues a TPM2_Shutdown command prior to loss of power, as required by the > * TPM 2.0 spec. Then, calls bus- and device- specific shutdown code. > * > * Return: always 0 (i.e. success) > */ > > I think this comment still refers to the ancient code here: > > https://elixir.bootlin.com/linux/v4.4.295/source/drivers/char/tpm/tpm-chip.c#L161 > > if (dev->bus && dev->bus->shutdown) > dev->bus->shutdown(dev); > else if (dev->driver && dev->driver->shutdown) > dev->driver->shutdown(dev); > Right, but that was because device_shutdown didn't use to call bus or driver shutdown routines if class->shutdown was defined. TPM is the class of devices that implements class->shutdown and that code above was removed and device_shutdown was made to call class and either bus/driver shutdown as well. See: Commit 7521621e600ae ("Do not disable driver and bus shutdown hook when class shutdown hook is set") > > >>> Fixes: 39d0099f9439 ("powerpc/pseries: Add shutdown() to vio_driver and >>> vio_bus") >> This patch left implementing each vio driver shutdown routine as an exercise for >> the respective maintainers, and instead just big hammers anything that doesn't >> have a shutdown routine by calling the driver->remove(). >> >> If tpm_class_shutdown() quiecses ibmvtpm enough implementing a no-op >> ibmvtpm->shutdown() with a comment saying so suffices. >> >> However, the generic TPM code is still introducing a bug that an attempt to call >> tpm_chip_unregister() after tpm_class_shutdown() will crash as mentioned above. > > > An alternative solution may be this here: Right, this is exactly what I was proposing in my last comment previously. > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index ddaeceb7e109..4cb908349b31 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -473,15 +473,8 @@ 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); > + if (chip->ops) > + tpm_class_shutdown(&chip->dev); > } > > static void tpm_del_legacy_sysfs(struct tpm_chip *chip) > > > I could post this as v2 ?! Let me know... Seeing as it is in TPM generic code and I'm not expert it would be nice if we got some input from Jarkko, or someone else with more experience in this area to make sure we aren't missing some other interaction. Otherwise, works for me. -Tyrel > > Stefan > > >>> 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; >>> + >> I wonder if a better fix would to have tpm_del_char_device() check for valid >> chip->ops and call tpm_class_shutdown() when the ops are still valid. Calling >> tpm_class_shutdown() allows for some code deduplication in tpm_del_char_device(). >> >> -Tyrel >> >>> tpm_clk_enable(chip); >>> >>> if (chip->locality == -1) { >>>
On 12/20/21 5:31 PM, Stefan Berger wrote: > > On 12/20/21 20:13, Jason Gunthorpe wrote: >> On Mon, Dec 20, 2021 at 08:05:58PM -0500, Stefan Berger wrote: >> >>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >>> index ddaeceb7e109..4cb908349b31 100644 >>> +++ b/drivers/char/tpm/tpm-chip.c >>> @@ -473,15 +473,8 @@ 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); >>> + if (chip->ops) >> ops cannot be read without locking > > This here could be an alternative: I still think code de-duplication is a good thing. Maybe combine the two approaches. Call tpm_class_shutdown from tpm_del_char_device and add a chip->ops check in tpm_class_shutdown to ensure we only do the shutdown when chip->ops is valid. -Tyrel > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index ddaeceb7e109..7772b475ebc0 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -474,7 +474,7 @@ static void tpm_del_char_device(struct tpm_chip *chip) > > /* Make the driver uncallable. */ > 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); > > Stefan > > >> >> Jason
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. /Jarkko
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? Stefan > > /Jarkko >
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
On Tue, Dec 21, 2021 at 09:01:06AM -0500, 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? It does change behaviour for all drivers, which is not acceptable for a bug fix. /Jarkko
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; + tpm_clk_enable(chip); if (chip->locality == -1) {
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(+)