| Message ID | 20190612084210.13562-1-gmazyland@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: Fix null pointer dereference on chip register error path | expand |
On Wed, 12 Jun 2019, Milan Broz wrote: > If clk_enable is not defined and chip initialization > is canceled code hits null dereference. > > Easily reproducible with vTPM init fail: > swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy > > BUG: kernel NULL pointer dereference, address: 00000000 > ... > Call Trace: > tpm_chip_start+0x9d/0xa0 [tpm] > tpm_chip_register+0x10/0x1a0 [tpm] > vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy] > process_one_work+0x214/0x5a0 > worker_thread+0x134/0x3e0 > ? process_one_work+0x5a0/0x5a0 > kthread+0xd4/0x100 > ? process_one_work+0x5a0/0x5a0 > ? kthread_park+0x90/0x90 > ret_from_fork+0x19/0x24 > > Signed-off-by: Milan Broz <gmazyland@gmail.com> > Cc: stable@vger.kernel.org Reviewed-by: James Morris <jamorris@linux.microsoft.com>
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.1.9, v4.19.50, v4.14.125, v4.9.181, v4.4.181.
v5.1.9: Build OK!
v4.19.50: Failed to apply! Possible dependencies:
100b16a6f290 ("tpm: sort objects in the Makefile")
29b47ce98759 ("tpm: move TPM space code out of tpm_transmit()")
412eb585587a ("tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter")
5faafbab77e3 ("tpm: remove @space from tpm_transmit()")
70a3199a7101 ("tpm: factor out tpm_get_timeouts()")
719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
899102bc4518 ("tpm2: add new tpm2 commands according to TCG 1.36")
9db7fe187c54 ("tpm: factor out tpm_startup function")
9e1b74a63f77 ("tpm: add support for nonblocking operation")
b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
b34b77a99b1a ("tpm: declare struct tpm_header")
c3465a370fb3 ("tpm: move tpm_validate_commmand() to tpm2-space.c")
c3d477a725ef ("tpm: add ptr to the tpm_space struct to file_priv")
c4df71d43a5b ("tpm: encapsulate tpm_dev_transmit()")
d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")
v4.14.125: Failed to apply! Possible dependencies:
09dd144f72e7 ("tpm: Add explicit endianness cast")
0bfb23746052 ("tpm: Move eventlog files to a subdirectory")
100b16a6f290 ("tpm: sort objects in the Makefile")
58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table")
67cb8e113ecd ("tpm: rename event log provider files")
719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
9b01b5356629 ("tpm: Move shared eventlog functions to common.c")
b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")
fd3ec3663718 ("tpm: move tpm_eventlog.h outside of drivers folder")
v4.9.181: Failed to apply! Possible dependencies:
02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
100b16a6f290 ("tpm: sort objects in the Makefile")
2528a64664f8 ("tpm: define a generic open() method for ascii & bios measurements")
719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
748935eeb72c ("tpm: have event log use the tpm_chip")
7518a21a9da3 ("tpm: drop tpm1_chip_register(/unregister)")
9b01b5356629 ("tpm: Move shared eventlog functions to common.c")
b1a9b7b602c5 ("tpm: replace symbolic permission with octal for securityfs files")
b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
cd9b7631a888 ("tpm: replace dynamically allocated bios_dir with a static array")
d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")
v4.4.181: Failed to apply! Possible dependencies:
02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
036bb38ffb3e ("tpm_tis: Ensure interrupts are disabled when the driver starts")
100b16a6f290 ("tpm: sort objects in the Makefile")
23d06ff700f5 ("tpm: drop tpm_atmel specific fields from tpm_vendor_specific")
25112048cd59 ("tpm: rework tpm_get_timeouts()")
41a5e1cf1fe1 ("tpm/tpm_tis: Split tpm_tis driver into a core and TCG TIS compliant phy")
4d627e672bd0 ("tpm_tis: Do not fall back to a hardcoded address for TPM2")
4eea703caaac ("tpm: drop 'iobase' from struct tpm_vendor_specific")
51dd43dff74b ("tpm_tis: Use devm_ioremap_resource")
55a889c2cb13 ("tpm_crb: Use the common ACPI definition of struct acpi_tpm2")
56671c893e0e ("tpm: drop 'locality' from struct tpm_vendor_specific")
570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
57dacc2b4ce5 ("tpm: tpm_tis: Share common data between phys")
719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code")
b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
d30b8e4f68ef ("tpm: cleanup tpm_tis_remove()")
d4956524f1b0 ("tpm: drop manufacturer_id from struct tpm_vendor_specific")
d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")
e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
ee1779840d09 ("tpm: drop 'base' from struct tpm_vendor_specific")
ef7b81dc7864 ("tpm_tis: Disable interrupt auto probing on a per-device basis")
How should we proceed with this patch?
--
Thanks,
Sasha
On 14/06/2019 23:56, Sasha Levin wrote: > Hi, > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: all It should be only # v5.1+ And seems I also forgot to add some cc for the original patch, sorry. The referenced patch is here https://lore.kernel.org/linux-integrity/20190612084210.13562-1-gmazyland@gmail.com/ Milan
Hi, is there any problem in this with the trivial patch below? I just get the same crash again with stable 5.1 kernel... Milan On 12/06/2019 10:42, Milan Broz wrote: > If clk_enable is not defined and chip initialization > is canceled code hits null dereference. > > Easily reproducible with vTPM init fail: > swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy > > BUG: kernel NULL pointer dereference, address: 00000000 > ... > Call Trace: > tpm_chip_start+0x9d/0xa0 [tpm] > tpm_chip_register+0x10/0x1a0 [tpm] > vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy] > process_one_work+0x214/0x5a0 > worker_thread+0x134/0x3e0 > ? process_one_work+0x5a0/0x5a0 > kthread+0xd4/0x100 > ? process_one_work+0x5a0/0x5a0 > ? kthread_park+0x90/0x90 > ret_from_fork+0x19/0x24 > > Signed-off-by: Milan Broz <gmazyland@gmail.com> > Cc: stable@vger.kernel.org > --- > drivers/char/tpm/tpm-chip.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 90325e1749fb..4c2af643d698 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -95,7 +95,8 @@ int tpm_chip_start(struct tpm_chip *chip) > if (chip->locality == -1) { > ret = tpm_request_locality(chip); > if (ret) { > - chip->ops->clk_enable(chip, false); > + if (chip->ops->clk_enable) > + chip->ops->clk_enable(chip, false); > return ret; > } > } >
On Fri, 2019-06-28 at 09:56 +0200, Milan Broz wrote: > Hi, > > is there any problem in this with the trivial patch below? > > I just get the same crash again with stable 5.1 kernel... > > Milan I'm sorry but I'm seeing this patch for the first time. /Jarkko
On Tue, Jul 02, 2019 at 09:37:51PM +0300, Jarkko Sakkinen wrote: > On Fri, 2019-06-28 at 09:56 +0200, Milan Broz wrote: > > Hi, > > > > is there any problem in this with the trivial patch below? > > > > I just get the same crash again with stable 5.1 kernel... > > > > Milan > > I'm sorry but I'm seeing this patch for the first time. OK, I finally reviewed your patch. Thank for finding this sever bug! I just have a few remarks. First of all, we need to add the necessary fixes tag to the patch, which will tell the commit ID that caused the crash and the one who we should blame: Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") It was a piece of a fairly large and complex refactorization [1] but it is not really a legit excuse and this is very unfortunate. I think it'd be better to take a different path how this will be fixed. Right after tpm_go_idle(), please add these: static void tpm_clk_enable(struct tpm_chip *chip) { if (chip->ops->clk_enable) chip->ops->clk_enable(chip); } static void tpm_tpm_clk_disable(struct tpm_chip *chip) { if (chip->ops->tpm_clk_disable) chip->ops->tpm_clk_disable(chip); } This is consistent with the other callbacks and gives better guarantees that a similar thing won't happen the next time when something happens to the call sites. This is what I should have done in my patch set to zero out the risk but failed to do that. You should categorically include the corresponding subsystem maintainers. Please check [2]. It is not like I would ignore any patches. It is more related to the risk of human error. Like many people, I filter any mailing list emails out of my inbox and go through them at some point. This will cause a random number of days of latency and with extremely bad luck I even might miss a patch completely. As a last remark put patch signed-off-by's and similar tags as last in your patch and put cc and fixes (in that order) before them. [1] https://lore.kernel.org/linux-integrity/20190205224723.19671-1-jarkko.sakkinen@linux.intel.com/ [2] https://www.kernel.org/doc/html/v5.1/process/submitting-patches.html#select-the-recipients-for-your-patch /Jarkko
On Thu, 2019-07-04 at 02:01 +0300, Jarkko Sakkinen wrote: > On Tue, Jul 02, 2019 at 09:37:51PM +0300, Jarkko Sakkinen wrote: > > On Fri, 2019-06-28 at 09:56 +0200, Milan Broz wrote: > > > Hi, > > > > > > is there any problem in this with the trivial patch below? > > > > > > I just get the same crash again with stable 5.1 kernel... > > > > > > Milan > > > > I'm sorry but I'm seeing this patch for the first time. > > OK, I finally reviewed your patch. Thank for finding this sever bug! I > just have a few remarks. > > First of all, we need to add the necessary fixes tag to the patch, which > will tell the commit ID that caused the crash and the one who we should > blame: > > Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()") > > It was a piece of a fairly large and complex refactorization [1] but it > is not really a legit excuse and this is very unfortunate. > > I think it'd be better to take a different path how this will be fixed. > > Right after tpm_go_idle(), please add these: > > static void tpm_clk_enable(struct tpm_chip *chip) > { > if (chip->ops->clk_enable) > chip->ops->clk_enable(chip); > } > > static void tpm_tpm_clk_disable(struct tpm_chip *chip) > { > if (chip->ops->tpm_clk_disable) > chip->ops->tpm_clk_disable(chip); > } > > This is consistent with the other callbacks and gives better guarantees > that a similar thing won't happen the next time when something happens > to the call sites. This is what I should have done in my patch set to > zero out the risk but failed to do that. > > You should categorically include the corresponding subsystem > maintainers. Please check [2]. It is not like I would ignore any > patches. It is more related to the risk of human error. > > Like many people, I filter any mailing list emails out of my inbox and > go through them at some point. This will cause a random number of days > of latency and with extremely bad luck I even might miss a patch > completely. > > As a last remark put patch signed-off-by's and similar tags as last in > your patch and put cc and fixes (in that order) before them. > > [1] > https://lore.kernel.org/linux-integrity/20190205224723.19671-1-jarkko.sakkinen@linux.intel.com/ > [2] > https://www.kernel.org/doc/html/v5.1/process/submitting-patches.html#select-the-recipients-for-your-patch So how should we work this one out? Do you want to create v2 or do I create a new patch and put reported-by tag. Both work for me. I just need to know this. /Jarkko
On 05/07/2019 19:52, Jarkko Sakkinen wrote: > > So how should we work this one out? Do you want to create v2 or do I > create a new patch and put reported-by tag. Both work for me. I just > need to know this. Please fix you mail filters, the patch was sent more than day ago. https://lore.kernel.org/linux-integrity/20190704072615.31143-1-gmazyland@gmail.com/T/#u Milan
On Fri, 2019-07-05 at 23:23 +0200, Milan Broz wrote: > On 05/07/2019 19:52, Jarkko Sakkinen wrote: > > So how should we work this one out? Do you want to create v2 or do I > > create a new patch and put reported-by tag. Both work for me. I just > > need to know this. > > Please fix you mail filters, the patch was sent more than day ago. > > https://lore.kernel.org/linux-integrity/20190704072615.31143-1-gmazyland@gmail.com/T/#u > > Milan Nothing wrong with my mail filters, it was actually waiting in my inbox. Just didn't notice it that it was part of the emails I downloaded to my laptop with fdm on Friday (because in rush to spend weekend). Didn't have time to go through all of them during that day. /Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 90325e1749fb..4c2af643d698 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -95,7 +95,8 @@ int tpm_chip_start(struct tpm_chip *chip) if (chip->locality == -1) { ret = tpm_request_locality(chip); if (ret) { - chip->ops->clk_enable(chip, false); + if (chip->ops->clk_enable) + chip->ops->clk_enable(chip, false); return ret; } }
If clk_enable is not defined and chip initialization is canceled code hits null dereference. Easily reproducible with vTPM init fail: swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy BUG: kernel NULL pointer dereference, address: 00000000 ... Call Trace: tpm_chip_start+0x9d/0xa0 [tpm] tpm_chip_register+0x10/0x1a0 [tpm] vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy] process_one_work+0x214/0x5a0 worker_thread+0x134/0x3e0 ? process_one_work+0x5a0/0x5a0 kthread+0xd4/0x100 ? process_one_work+0x5a0/0x5a0 ? kthread_park+0x90/0x90 ret_from_fork+0x19/0x24 Signed-off-by: Milan Broz <gmazyland@gmail.com> Cc: stable@vger.kernel.org --- drivers/char/tpm/tpm-chip.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)