In tpm_del_char_device() make sure that chip->ops is still valid. This check is needed since in case of a system shutdown tpm_class_shutdown() has already been called and set chip->ops to NULL. This leads to a NULL pointer access as soon as tpm_del_char_device() tries to access chip->ops in case of TPM 2.
Fixes: dcbeab1946454 ("tpm: fix crash in tpm_tis deinitialization") Cc: stable@vger.kernel.org Signed-off-by: Lino Sanfilippo LinoSanfilippo@gmx.de --- drivers/char/tpm/tpm-chip.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e109..ed1fb5d82caf 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -474,13 +474,19 @@ 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 (!tpm_chip_start(chip)) { - tpm2_shutdown(chip, TPM2_SU_CLEAR); - tpm_chip_stop(chip); + /* Check if chip->ops is still valid since in case of a shutdown + * tpm_class_shutdown() has already sent the TPM2_Shutdown command + * and set chip->ops to NULL. + */ + if (chip->ops) { + 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; } - chip->ops = NULL; up_write(&chip->ops_sem); }
base-commit: a3fa7a101dcff93791d1b1bdb3affcad1410c8c1
On Fri, 2021-09-10 at 20:04 +0200, Lino Sanfilippo wrote:
In tpm_del_char_device() make sure that chip->ops is still valid. This check is needed since in case of a system shutdown tpm_class_shutdown() has already been called and set chip->ops to NULL. This leads to a NULL pointer access as soon as tpm_del_char_device() tries to access chip->ops in case of TPM 2.
Fixes: dcbeab1946454 ("tpm: fix crash in tpm_tis deinitialization") Cc: stable@vger.kernel.org Signed-off-by: Lino Sanfilippo LinoSanfilippo@gmx.de
Have you been able to reproduce this in some environment?
/Jarkko
Hi,
Gesendet: Montag, 13. September 2021 um 22:25 Uhr Von: "Jarkko Sakkinen" jarkko@kernel.org An: "Lino Sanfilippo" LinoSanfilippo@gmx.de, peterhuewe@gmx.de, jgg@ziepe.ca Cc: p.rosenberger@kunbus.com, linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Betreff: Re: [PATCH] tpm: fix potential NULL pointer access in tpm_del_char_device()
On Fri, 2021-09-10 at 20:04 +0200, Lino Sanfilippo wrote:
In tpm_del_char_device() make sure that chip->ops is still valid. This check is needed since in case of a system shutdown tpm_class_shutdown() has already been called and set chip->ops to NULL. This leads to a NULL pointer access as soon as tpm_del_char_device() tries to access chip->ops in case of TPM 2.
Fixes: dcbeab1946454 ("tpm: fix crash in tpm_tis deinitialization") Cc: stable@vger.kernel.org Signed-off-by: Lino Sanfilippo LinoSanfilippo@gmx.de
Have you been able to reproduce this in some environment?
/Jarkko
Yes, this bug is reproducable on my system that is running a 5.10 raspberry kernel. I use a SLB 9670 which is connected via SPI.
Regards, Lino
On Mon, 2021-09-13 at 22:53 +0200, Lino Sanfilippo wrote:
Hi,
Gesendet: Montag, 13. September 2021 um 22:25 Uhr Von: "Jarkko Sakkinen" jarkko@kernel.org An: "Lino Sanfilippo" LinoSanfilippo@gmx.de, peterhuewe@gmx.de, jgg@ziepe.ca Cc: p.rosenberger@kunbus.com, linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Betreff: Re: [PATCH] tpm: fix potential NULL pointer access in tpm_del_char_device()
On Fri, 2021-09-10 at 20:04 +0200, Lino Sanfilippo wrote:
In tpm_del_char_device() make sure that chip->ops is still valid. This check is needed since in case of a system shutdown tpm_class_shutdown() has already been called and set chip->ops to NULL. This leads to a NULL pointer access as soon as tpm_del_char_device() tries to access chip->ops in case of TPM 2.
Fixes: dcbeab1946454 ("tpm: fix crash in tpm_tis deinitialization") Cc: stable@vger.kernel.org Signed-off-by: Lino Sanfilippo LinoSanfilippo@gmx.de
Have you been able to reproduce this in some environment?
/Jarkko
Yes, this bug is reproducable on my system that is running a 5.10 raspberry kernel. I use a SLB 9670 which is connected via SPI.
Can you confirm that the lates mainline kernel has also this issue here? That is lacking in this fix.
It's obvious that the issue does not scale to every system, so it would nice to know the difference that triggers the issue, before applying this, and it also needs to be documented to the commit message.
/Jarkko
Hi,
On 14.09.21 at 02:31, Jarkko Sakkinen wrote:
On Mon, 2021-09-13 at 22:53 +0200, Lino Sanfilippo wrote:
Hi,
Gesendet: Montag, 13. September 2021 um 22:25 Uhr Von: "Jarkko Sakkinen" jarkko@kernel.org An: "Lino Sanfilippo" LinoSanfilippo@gmx.de, peterhuewe@gmx.de, jgg@ziepe.ca Cc: p.rosenberger@kunbus.com, linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Betreff: Re: [PATCH] tpm: fix potential NULL pointer access in tpm_del_char_device()
On Fri, 2021-09-10 at 20:04 +0200, Lino Sanfilippo wrote:
In tpm_del_char_device() make sure that chip->ops is still valid. This check is needed since in case of a system shutdown tpm_class_shutdown() has already been called and set chip->ops to NULL. This leads to a NULL pointer access as soon as tpm_del_char_device() tries to access chip->ops in case of TPM 2.
Fixes: dcbeab1946454 ("tpm: fix crash in tpm_tis deinitialization") Cc: stable@vger.kernel.org Signed-off-by: Lino Sanfilippo LinoSanfilippo@gmx.de
Have you been able to reproduce this in some environment?
/Jarkko
Yes, this bug is reproducable on my system that is running a 5.10 raspberry kernel. I use a SLB 9670 which is connected via SPI.
Can you confirm that the lates mainline kernel has also this issue here? That is lacking in this fix.
It's obvious that the issue does not scale to every system, so it would nice to know the difference that triggers the issue, before applying this, and it also needs to be documented to the commit message.
Sorry for the long delay in replying. I have tried to get a recent mainline kernel running on my hardware but so far without success. Concerning the circumstances under which this bug triggers and if this also applies to the current mainline code please take a look at the kernel dump on my system:
[ 174.078277] 8<--- cut here --- [ 174.078288] Unable to handle kernel NULL pointer dereference at virtual address 00000034 [ 174.078293] pgd = 557a5fc9 [ 174.078300] [00000034] *pgd=031cf003, *pmd=00000000 [ 174.078317] Internal error: Oops: 206 [#1] SMP ARM [ 174.078323] Modules linked in: tpm_tis_spi tpm_tis_core tpm spidev gpio_pca953x mcp320x rtc_pcf2127 industrialio regmap_i2c regmap_spi 8021q garp stp llc ftdi_sio6 [ 174.078441] CPU: 3 PID: 1 Comm: systemd-shutdow Tainted: G WC 5.10.27-rt36-C4LS+ #1 [ 174.078448] Hardware name: BCM2835 [ 174.078451] PC is at tpm_chip_start+0x1c/0xc0 [tpm] [ 174.078492] LR is at tpm_chip_unregister+0xc0/0xe0 [tpm] [ 174.078525] pc : [<bf244080>] lr : [<bf2447c8>] psr: 20000013 [ 174.078529] sp : c1903c38 ip : c1903c50 fp : c1903c4c [ 174.078533] r10: c1aca054 r9 : c0e77b28 r8 : c311c000 [ 174.078537] r7 : 00000000 r6 : bf262010 r5 : c323fbf8 r4 : c323f800 [ 174.078541] r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : c323f800 [ 174.078546] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 174.078553] Control: 30c5383d Table: 02a47980 DAC: f7bd3313 [ 174.078556] Process systemd-shutdow (pid: 1, stack limit = 0xa0551b1d) [ 174.078561] Stack: (0xc1903c38 to 0xc1904000) [ 174.078566] 3c20: c323f800 c323fbf8 [ 174.078574] 3c40: c1903c64 c1903c50 bf2447c8 bf244070 c323f800 c3498800 c1903c7c c1903c68 [ 174.078581] 3c60: bf260190 bf244714 c3498800 c3498800 c1903c94 c1903c80 c08b9fa8 bf26017c [ 174.078588] 3c80: c3498800 00000000 c1903cb4 c1903c98 c0862b20 c08b9f7c c1aaf730 c3498800 [ 174.078595] 3ca0: c12fc9d0 00000000 c1903cc4 c1903cb8 c0862bf4 c0862a1c c1903ce4 c1903cc8 [ 174.078602] 3cc0: c08612d0 c0862be0 c3498800 00005744 c08ba298 00000000 c1903d2c c1903ce8 [ 174.078609] 3ce0: c085c61c c0861200 ffffe000 c13404c0 c0781f40 c0e77b28 c1903d2c c1205048 [ 174.078616] 3d00: c0332c3c c3498800 00000000 c08ba298 c13fdd7c c1356018 c0e77b28 c1aca054 [ 174.078623] 3d20: c1903d44 c1903d30 c085c8d0 c085c498 c3498800 00000000 c1903d5c c1903d48 [ 174.078630] 3d40: c08ba294 c085c8c0 c311c000 00000000 c1903d6c c1903d60 c08ba2b0 c08ba25c [ 174.078638] 3d60: c1903d9c c1903d70 c085bb90 c08ba2a4 c1903d8c c369a080 c369a114 c1205048 [ 174.078645] 3d80: c1903da4 c311c000 c311c000 00000000 c1903dbc c1903da0 c08ba748 c085bb2c [ 174.078651] 3da0: c311c380 c311c000 00000000 c13fdd7c c1903ddc c1903dc0 bf168534 c08ba718 [ 174.078659] 3dc0: c1aca000 c1a75010 c1aca010 c13fdd7c c1903df4 c1903de0 bf168588 bf16850c [ 174.078666] 3de0: c1aca014 c1a75010 c1903e04 c1903df8 c0863ca0 bf168578 c1903e3c c1903e08 [ 174.078673] 3e00: c085fc90 c0863c80 c1903e3c c0e77b18 c0248888 00000000 00000000 8855a600 [ 174.078680] 3e20: c120f1cc fee1dead c1902000 00000058 c1903e4c c1903e40 c0249eb0 c085fb00 [ 174.078687] 3e40: c1903e64 c1903e50 c0249fa0 c0249e78 01234567 00000000 c1903f94 c1903e68 [ 174.078694] 3e60: c024a244 c0249f90 c1903ed4 c2ec5180 00000024 c1903f58 00000005 c0441f50 [ 174.078701] 3e80: c1903ec4 c1903e90 c0441d94 c0498350 00000000 c1903ea0 c0739fa4 00000024 [ 174.078708] 3ea0: c2ec5180 c1903f58 c1903ed4 c2ec5180 00000005 00000000 c1903f4c c1903ec8 [ 174.078715] 3ec0: c0441f50 c0425938 c1903ed0 c1903ed4 00000000 00000005 00000000 00000024 [ 174.078722] 3ee0: c1903eec 00000005 c020007c bec81250 00000004 bec81f62 00000010 bec81264 [ 174.078729] 3f00: 00000005 bec8131c 0000000a b6d0ef50 00000001 c0200e70 ffffe000 c13404c0 [ 174.078736] 3f20: 00000000 c04673e8 c1903f4c c1205048 c2ec5180 bec8128c 00000000 00000000 [ 174.078743] 3f40: c1903f94 c1903f50 c04420d0 c0441eb4 00000000 c13404c0 00000000 00000000 [ 174.078750] 3f60: c1903f94 c1205048 c0332c3c c1205048 bec8131c 00000000 00000000 00000000 [ 174.078757] 3f80: 00000058 c0200204 c1903fa4 c1903f98 c024a398 c024a13c 00000000 c1903fa8 [ 174.078764] 3fa0: c0200040 c024a38c 00000000 00000000 fee1dead 28121969 01234567 8855a600 [ 174.078771] 3fc0: 00000000 00000000 00000000 00000058 00000fff bec81be8 00000000 00456b80 [ 174.078778] 3fe0: 00468e3c bec81b68 004534a8 b6e4ba38 60000010 fee1dead 00000000 00000000 [ 174.078782] Backtrace: [ 174.078786] [<bf244064>] (tpm_chip_start [tpm]) from [<bf2447c8>] (tpm_chip_unregister+0xc0/0xe0 [tpm]) [ 174.078853] r5:c323fbf8 r4:c323f800 [ 174.078855] [<bf244708>] (tpm_chip_unregister [tpm]) from [<bf260190>] (tpm_tis_spi_remove+0x20/0x30 [tpm_tis_spi]) [ 174.078899] r5:c3498800 r4:c323f800 [ 174.078901] [<bf260170>] (tpm_tis_spi_remove [tpm_tis_spi]) from [<c08b9fa8>] (spi_drv_remove+0x38/0x50) [ 174.078923] r5:c3498800 r4:c3498800 [ 174.078926] [<c08b9f70>] (spi_drv_remove) from [<c0862b20>] (device_release_driver_internal+0x110/0x1c4) [ 174.078942] r5:00000000 r4:c3498800 [ 174.078945] [<c0862a10>] (device_release_driver_internal) from [<c0862bf4>] (device_release_driver+0x20/0x24) [ 174.078959] r7:00000000 r6:c12fc9d0 r5:c3498800 r4:c1aaf730 [ 174.078962] [<c0862bd4>] (device_release_driver) from [<c08612d0>] (bus_remove_device+0xdc/0x108) [ 174.078973] [<c08611f4>] (bus_remove_device) from [<c085c61c>] (device_del+0x190/0x428) [ 174.078989] r7:00000000 r6:c08ba298 r5:00005744 r4:c3498800 [ 174.078992] [<c085c48c>] (device_del) from [<c085c8d0>] (device_unregister+0x1c/0x30) [ 174.079009] r10:c1aca054 r9:c0e77b28 r8:c1356018 r7:c13fdd7c r6:c08ba298 r5:00000000 [ 174.079013] r4:c3498800 [ 174.079015] [<c085c8b4>] (device_unregister) from [<c08ba294>] (spi_unregister_device+0x44/0x48) [ 174.079030] r5:00000000 r4:c3498800 [ 174.079033] [<c08ba250>] (spi_unregister_device) from [<c08ba2b0>] (__unregister+0x18/0x20) [ 174.079048] r5:00000000 r4:c311c000 [ 174.079050] [<c08ba298>] (__unregister) from [<c085bb90>] (device_for_each_child+0x70/0xb4) [ 174.079064] [<c085bb20>] (device_for_each_child) from [<c08ba748>] (spi_unregister_controller+0x3c/0x134) [ 174.079081] r6:00000000 r5:c311c000 r4:c311c000 [ 174.079083] [<c08ba70c>] (spi_unregister_controller) from [<bf168534>] (bcm2835_spi_remove+0x34/0x6c [spi_bcm2835]) [ 174.079104] r7:c13fdd7c r6:00000000 r5:c311c000 r4:c311c380 [ 174.079107] [<bf168500>] (bcm2835_spi_remove [spi_bcm2835]) from [<bf168588>] (bcm2835_spi_shutdown+0x1c/0x38 [spi_bcm2835]) [ 174.079130] r7:c13fdd7c r6:c1aca010 r5:c1a75010 r4:c1aca000 [ 174.079132] [<bf16856c>] (bcm2835_spi_shutdown [spi_bcm2835]) from [<c0863ca0>] (platform_drv_shutdown+0x2c/0x30) [ 174.079150] r5:c1a75010 r4:c1aca014 [ 174.079153] [<c0863c74>] (platform_drv_shutdown) from [<c085fc90>] (device_shutdown+0x19c/0x24c) [ 174.079164] [<c085faf4>] (device_shutdown) from [<c0249eb0>] (kernel_restart_prepare+0x44/0x48) [ 174.079183] r10:00000058 r9:c1902000 r8:fee1dead r7:c120f1cc r6:8855a600 r5:00000000 [ 174.079186] r4:00000000 [ 174.079189] [<c0249e6c>] (kernel_restart_prepare) from [<c0249fa0>] (kernel_restart+0x1c/0x60) [ 174.079203] [<c0249f84>] (kernel_restart) from [<c024a244>] (__do_sys_reboot+0x114/0x1f8) [ 174.079218] r5:00000000 r4:01234567 [ 174.079221] [<c024a130>] (__do_sys_reboot) from [<c024a398>] (sys_reboot+0x18/0x1c) [ 174.079238] r8:c0200204 r7:00000058 r6:00000000 r5:00000000 r4:00000000 [ 174.079241] [<c024a380>] (sys_reboot) from [<c0200040>] (ret_fast_syscall+0x0/0x28) [ 174.079254] Exception stack(0xc1903fa8 to 0xc1903ff0) [ 174.079260] 3fa0: 00000000 00000000 fee1dead 28121969 01234567 8855a600 [ 174.079267] 3fc0: 00000000 00000000 00000000 00000058 00000fff bec81be8 00000000 00456b80 [ 174.079273] 3fe0: 00468e3c bec81b68 004534a8 b6e4ba38 [ 174.079280] Code: e52de004 e8bd4000 e5903410 e1a04000 (e5932034) [ 174.079285] ---[ end trace 33e1042219f38210 ]--- [ 174.879428] Kernel panic - not syncing: [ 174.879432] Attempted to kill init! exitcode=0x0000000b
Note that before this bug happens all pre_shutdown handlers have already been executed including tpm_class_shutdown() which sets chip->ops to NULL.
So this bug is triggered when the bcm2835 drivers shutdown() function is called since this driver does something quite unusual: it unregisters the spi controller in its shutdown() handler.
This eventually results in a call to tpm_del_char_device(). This is not problematic unless we have TPM2, since in this case tpm_chip_start() is called and chip->ops is accessed again, resulting in the NULL pointer access.
So to make it short the combination of - the tpm chips pre_shutdown handler setting chip->ops to NULL - the use of the bcm2835 driver with the spi controller deregistration in its shutdown function - the use of TPM 2
results in the situation in which this bug can trigger.
AFAICS this is still the case in the mainline code. Also note that with the freescale LPSPI controller driver there is another driver that unregisters the spi controller in its shutdown function.
Please let me know if you need further information.
Regards, Lino
On 24.09.21 at 15:33, Jason Gunthorpe wrote:
On Fri, Sep 24, 2021 at 03:29:46PM +0200, Lino Sanfilippo wrote:
So this bug is triggered when the bcm2835 drivers shutdown() function is called since this driver does something quite unusual: it unregisters the spi controller in its shutdown() handler.
This seems wrong
Jason
Unregistering the SPI controller during shutdown is only a side-effect of calling bcm2835_spi_remove() in the shutdown handler:
static void bcm2835_spi_shutdown(struct platform_device *pdev) { int ret;
ret = bcm2835_spi_remove(pdev); if (ret) dev_err(&pdev->dev, "failed to shutdown\n"); }
Regards, Lino
On Fri, Sep 24, 2021 at 04:17:52PM +0200, Lino Sanfilippo wrote:
On 24.09.21 at 15:33, Jason Gunthorpe wrote:
On Fri, Sep 24, 2021 at 03:29:46PM +0200, Lino Sanfilippo wrote:
So this bug is triggered when the bcm2835 drivers shutdown() function is called since this driver does something quite unusual: it unregisters the spi controller in its shutdown() handler.
This seems wrong
Jason
Unregistering the SPI controller during shutdown is only a side-effect of calling bcm2835_spi_remove() in the shutdown handler:
static void bcm2835_spi_shutdown(struct platform_device *pdev) { int ret;
ret = bcm2835_spi_remove(pdev); if (ret) dev_err(&pdev->dev, "failed to shutdown\n"); }
That's wrong, the shutdown handler is only supposed to make the HW stop doing DMA and interrupts so we can have a clean transition to kexec/etc
It should not be manipulating other state.
Jason
Hi,
Gesendet: Freitag, 24. September 2021 um 16:20 Uhr Von: "Jason Gunthorpe" jgg@ziepe.ca An: "Lino Sanfilippo" LinoSanfilippo@gmx.de Cc: "Jarkko Sakkinen" jarkko@kernel.org, peterhuewe@gmx.de, p.rosenberger@kunbus.com, linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Betreff: Re: [PATCH] tpm: fix potential NULL pointer access in tpm_del_char_device()
On Fri, Sep 24, 2021 at 04:17:52PM +0200, Lino Sanfilippo wrote:
On 24.09.21 at 15:33, Jason Gunthorpe wrote:
On Fri, Sep 24, 2021 at 03:29:46PM +0200, Lino Sanfilippo wrote:
So this bug is triggered when the bcm2835 drivers shutdown() function is called since this driver does something quite unusual: it unregisters the spi controller in its shutdown() handler.
This seems wrong
Jason
Unregistering the SPI controller during shutdown is only a side-effect of calling bcm2835_spi_remove() in the shutdown handler:
static void bcm2835_spi_shutdown(struct platform_device *pdev) { int ret;
ret = bcm2835_spi_remove(pdev); if (ret) dev_err(&pdev->dev, "failed to shutdown\n"); }
That's wrong, the shutdown handler is only supposed to make the HW stop doing DMA and interrupts so we can have a clean transition to kexec/etc
It should not be manipulating other state.
I created another patch that fixes the issue in the BCM2835 driver instead (see https://marc.info/?l=linux-spi&m=163285906725366&w=2).
However I still think that the fix I proposed for TPM is valueable, because it saves us from any SPI controller driver that does not know/care about the issue that is caused in TPM by unregistering the controller in the shutdown handler. Note that the freescale DSPI driver is another candidate that behaves errorneous in this way.
Regards, Lino
linux-stable-mirror@lists.linaro.org
-
Jarkko Sakkinen -
Jason Gunthorpe -
Lino Sanfilippo