Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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 0x0060
> > > Faulting instruction address: 0xc099a06c
> > > Oops: Kernel access of bad area, sig: 11 [#1]
> > > ...
> > > NIP [c099a06c] tpm_chip_start+0x2c/0x140
> > > LR [c099a808] tpm_chip_unregister+0x108/0x170
> > > Call Trace:
> > > [c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8
> > > (unreliable)
> > > [c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
> > > [c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
> > > [c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
> > > [c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
> > > [c000188bfbc0] [c0196e14] 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
> > > ---
> > > 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
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 0x0060
Faulting instruction address: 0xc099a06c
Oops: Kernel access of bad area, sig: 11 [#1]
...
NIP [c099a06c] tpm_chip_start+0x2c/0x140
LR [c099a808] tpm_chip_unregister+0x108/0x170
Call Trace:
[c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8
(unreliable)
[c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
[c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
[c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
[c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
[c000188bfbc0] [c0196e14] 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
---
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(>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(_lock);
/* Make the driver uncallable. */
- down_write(>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(>ops_sem);
+ tpm_class_shutdown(>dev);
}
static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
Stefan
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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 0x0060
Faulting instruction address: 0xc099a06c
Oops: Kernel access of bad area, sig: 11 [#1]
...
NIP [c099a06c] tpm_chip_start+0x2c/0x140
LR [c099a808] tpm_chip_unregister+0x108/0x170
Call Trace:
[c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
[c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
[c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
[c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
[c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
[c000188bfbc0] [c0196e14] 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
---
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
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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 0x0060
> Faulting instruction address: 0xc099a06c
> Oops: Kernel access of bad area, sig: 11 [#1]
> ...
> NIP [c099a06c] tpm_chip_start+0x2c/0x140
> LR [c099a808] tpm_chip_unregister+0x108/0x170
> Call Trace:
> [c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
> [c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
> [c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
> [c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
> [c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
> [c000188bfbc0] [c0196e14] 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
> ---
> 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
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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(_lock);
>>>
>>> /* Make the driver uncallable. */
>>> - down_write(>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(>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(>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
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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 0x0060
>>> Faulting instruction address: 0xc099a06c
>>> Oops: Kernel access of bad area, sig: 11 [#1]
>>> ...
>>> NIP [c099a06c] tpm_chip_start+0x2c/0x140
>>> LR [c099a808] tpm_chip_unregister+0x108/0x170
>>> Call Trace:
>>> [c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
>>> [c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
>>> [c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
>>> [c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
>>> [c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
>>> [c000188bfbc0] [c0196e14] 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(_lock);
>
> /* Make the driver uncallable. */
> - down_write(>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(>ops_sem);
> + if (chip->ops)
> + tpm_class_shutdown(>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
>
>
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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(_lock);
/* Make the driver uncallable. */
- down_write(>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(>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(>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
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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(_lock);
>
> /* Make the driver uncallable. */
> - down_write(>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(>ops_sem);
> + if (chip->ops)
ops cannot be read without locking
Jason
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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 0x0060
Faulting instruction address: 0xc099a06c
Oops: Kernel access of bad area, sig: 11 [#1]
...
NIP [c099a06c] tpm_chip_start+0x2c/0x140
LR [c099a808] tpm_chip_unregister+0x108/0x170
Call Trace:
[c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
[c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
[c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
[c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
[c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
[c000188bfbc0] [c0196e14] 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(_lock);
/* Make the driver uncallable. */
- down_write(>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(>ops_sem);
+ if (chip->ops)
+ tpm_class_shutdown(>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
---
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) {
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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 0x0060
> Faulting instruction address: 0xc099a06c
> Oops: Kernel access of bad area, sig: 11 [#1]
> ...
> NIP [c099a06c] tpm_chip_start+0x2c/0x140
> LR [c099a808] tpm_chip_unregister+0x108/0x170
> Call Trace:
> [c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
> [c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
> [c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
> [c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
> [c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
> [c000188bfbc0] [c0196e14] 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
> ---
> 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) {
>
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
> On 12-Dec-2021, at 6:58 AM, 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 0x0060
> Faulting instruction address: 0xc099a06c
> Oops: Kernel access of bad area, sig: 11 [#1]
> ...
> NIP [c099a06c] tpm_chip_start+0x2c/0x140
> LR [c099a808] tpm_chip_unregister+0x108/0x170
> Call Trace:
> [c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
> [c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
> [c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
> [c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
> [c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
> [c000188bfbc0] [c0196e14] 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
> ---
With the patch applied, kexec with vTPM works as expected.
Tested-by: Sachin Sant
Thanks
-Sachin
[PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
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 0x0060
Faulting instruction address: 0xc099a06c
Oops: Kernel access of bad area, sig: 11 [#1]
...
NIP [c099a06c] tpm_chip_start+0x2c/0x140
LR [c099a808] tpm_chip_unregister+0x108/0x170
Call Trace:
[c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
[c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
[c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
[c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
[c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
[c000188bfbc0] [c0196e14] 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
---
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;
+
tpm_clk_enable(chip);
if (chip->locality == -1) {
--
2.31.1
