New issue
Advanced search Search tips

Issue 763394 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 764001



Sign in to add a comment

tpm: shutdown / sysfs race condition affecting TPM2 devices in chromeos-4.12

Project Member Reported by groeck@chromium.org, Sep 8 2017

Issue description

chromeos-4.12 and later use the upstream TPM code. That code handles shutdown differently than chromeos-4.4 and earlier. The upstream solution, implemented with commit d1bd4a792d3 ("tpm: Issue a TPM2_Shutdown for TPM2 devices") does not support sysfs. From the commit log:

"NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs, and sysfs relies on implicit locking on chip->ops, it is not safe to allow this code to run in TPM1, or to add sysfs support to TPM2, until that locking is made explicit."

ChromeOS uses sysfs for TPM2, thus the problem affects us.

 
Blocking: 764001

Comment 2 by groeck@chromium.org, Nov 16 2017

Owner: groeck@chromium.org
Due to the way shutdown is handled upstream, this causes a crash on reboot.
Problem is that chip->ops is cleared in the shutdown handler while user space is still accessing the chip.

<1>[   46.024053] BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
<1>[   46.024063] IP: tpm_transmit+0x16c/0x380
<6>[   46.024064] PGD 0 P4D 0 
<4>[   46.024067] Oops: 0000 [#1] PREEMPT SMP
<4>[   46.024070] Modules linked in: cmac rfcomm uinput snd_soc_kbl_rt5663_rt5514_max98927 snd_soc_dmic snd_soc_hdac_hdmi iwlmvm mac80211 xt_nat snd_soc_skl snd_soc_skl_ipc snd_soc_sst_ipc lzo lzo_compress snd_soc_sst_dsp snd_soc_sst_match snd_hda_ext_core snd_hda_core iwlwifi zram bridge stp llc snd_soc_rt5514 snd_soc_rt5663 snd_soc_rt5514_spi snd_soc_max98927 snd_soc_rl6231 ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat xt_mark fuse cdc_ether usbnet r8152 mii btusb btrtl btbcm btintel uvcvideo bluetooth videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core usb_serial_simple ecdh_generic cfg80211 joydev hid_multitouch iio_trig_sysfs ip6table_filter cros_ec_light_prox cros_ec_sensors cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf industrialio
<4>[   46.024109] CPU: 2 PID: 2706 Comm: cryptohomed Not tainted 4.14.0-01601-g863d7b87fc731 #1
<4>[   46.024110] Hardware name: Google Eve/Eve, BIOS Google_Eve.9584.95.0 09/27/2017
<4>[   46.024112] task: ffff975814ca0f00 task.stack: ffffad5f016bc000
<4>[   46.024114] RIP: 0010:tpm_transmit+0x16c/0x380
<4>[   46.024115] RSP: 0018:ffffad5f016bfbc8 EFLAGS: 00010202
<4>[   46.024117] RAX: 0000000000000000 RBX: ffff97586b495000 RCX: 0000000000000000
<4>[   46.024118] RDX: fffffff314ca0f00 RSI: 0000000000000202 RDI: ffff97586b770108
<4>[   46.024119] RBP: ffffad5f016bfc20 R08: 0000000000000000 R09: 00000000000003e2
<4>[   46.024120] R10: ffffad5f016bfba0 R11: 000000000000003d R12: ffffad5f016bfc6c
<4>[   46.024121] R13: 000000000000017a R14: 0000000000000000 R15: 000000000000008c
<4>[   46.024123] FS:  0000791ef1b7c7c0(0000) GS:ffff97587ed00000(0000) knlGS:0000000000000000
<4>[   46.024124] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[   46.024125] CR2: 0000000000000038 CR3: 0000000435c6e004 CR4: 00000000003606e0
<4>[   46.024127] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
<4>[   46.024128] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
<4>[   46.024129] Call Trace:
<4>[   46.024133]  tpm_transmit_cmd+0x25/0x70
<4>[   46.024135]  tpm2_get_tpm_pt+0x79/0xa4
<4>[   46.024138]  ? avc_has_perm_noaudit+0xb1/0xd7
<4>[   46.024140]  tpm2_prop_flag_show+0x38/0x7c
<4>[   46.024143]  dev_attr_show+0x25/0x49
<4>[   46.024146]  sysfs_kf_seq_show+0x83/0xcf
<4>[   46.024147]  kernfs_seq_show+0x27/0x29
<4>[   46.024150]  seq_read+0x190/0x35b
<4>[   46.024152]  kernfs_fop_read+0x3a/0x156
<4>[   46.024154]  __vfs_read+0x35/0xc0
<4>[   46.024157]  ? fsnotify_perm+0x64/0x6f
<4>[   46.024159]  ? security_file_permission+0x3b/0x42
<4>[   46.024160]  vfs_read+0xa9/0xe5
<4>[   46.024162]  SyS_read+0x5f/0xa3
<4>[   46.024165]  entry_SYSCALL_64_fastpath+0x27/0xa8
<4>[   46.024167] RIP: 0033:0x791ef2e16e6d
<4>[   46.024168] RSP: 002b:00007ffe094a2f30 EFLAGS: 00000293 ORIG_RAX: 0000000000000000
<4>[   46.024169] RAX: ffffffffffffffda RBX: 0000791ef30d2bd8 RCX: 0000791ef2e16e6d
<4>[   46.024171] RDX: 0000000000010000 RSI: 00005ce6b3de7960 RDI: 0000000000000009
<4>[   46.024172] RBP: 00007ffe094a2f40 R08: 0000000000000150 R09: 0000000000001160
<4>[   46.024173] R10: 0000000000001011 R11: 0000000000000293 R12: 0000000000002709
<4>[   46.024174] R13: 0000000000010010 R14: 0000791ef30d2b80 R15: 00005ce6b3de7960
<4>[   46.024176] Code: ff 74 0a be 04 00 00 00 e8 61 d7 0f 00 83 bb d8 08 00 00 ff 0f 94 45 b7 f6 45 b0 02 75 2e 80 7d b7 00 74 28 48 8b 83 70 07 00 00 <48> 8b 40 38 48 85 c0 74 18 31 f6 48 89 df ff d0 41 89 c7 85 c0 
<1>[   46.024195] RIP: tpm_transmit+0x16c/0x380 RSP: ffffad5f016bfbc8
<4>[   46.024196] CR2: 0000000000000038
<4>[   46.024197] ---[ end trace bcc1176e94e0bbfc ]---
<0>[   46.026361] Kernel panic - not syncing: Fatal exception
<0>[   46.026394] Kernel Offset: 0x27000000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)

Labels: Kernel-4.14
Oh, so we are seeing it in real life for 4.12. Is there a particular test that fails? 

I'd expect that normally in most cases nothing reads tpm sysfs during shutdown. (Unless we cause it intentionally.) Plus, upstream tpm 2.0 doesn't even have anything in sysfs. And in chromeos case, iirc, the only code that deals with tpm sysfs are some factory scripts. So, do we know who tries to read it during shutdown and why?
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 16 2017

Labels: merge-merged-chromeos-4.14-tpm
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel-next/+/d972ccf2b8f916ba30c23481d745b954d64c8d4b

commit d972ccf2b8f916ba30c23481d745b954d64c8d4b
Author: Guenter Roeck <groeck@chromium.org>
Date: Thu Nov 16 21:42:39 2017

CHROMIUM: tpm: Protect tpm2 sysfs accesses against shutdown

The tpm shutdown code set chip->ops to NULL to prevent further chip access.
This happens before the device is removed. Userspace can thus still access the
chip, which causes a crash due to NULL pointer accesses.

BUG= chromium:763394 
TEST=Run TPM test cases

Change-Id: Ib2add1a4acceff53ce077ee223ecbdf905f2b451
Signed-off-by: Guenter Roeck <groeck@chromium.org>

[modify] https://crrev.com/d972ccf2b8f916ba30c23481d745b954d64c8d4b/drivers/char/tpm/tpm-sysfs.c

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel-next/+/ee5eeca084a53cbb233f16d4d6772716d3c05f16

commit ee5eeca084a53cbb233f16d4d6772716d3c05f16
Author: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date: Thu Nov 16 20:29:12 2017

UPSTREAM: tpm: migrate pubek_show to struct tpm_buf

Migrated pubek_show to struct tpm_buf and cleaned up its implementation.
Previously the output parameter structure was declared but left
completely unused. Now it is used to refer different fields of the
output. We can move it to tpm-sysfs.c as it does not have any use
outside of that file.

BUG= chromium:763394 
TEST=Run TPM test cases

Change-Id: I454fb7a8f0225e192c8822de17bf95de0c20a1f4
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from upstream commit da379f3c1db0)

[modify] https://crrev.com/ee5eeca084a53cbb233f16d4d6772716d3c05f16/drivers/char/tpm/tpm-sysfs.c
[modify] https://crrev.com/ee5eeca084a53cbb233f16d4d6772716d3c05f16/drivers/char/tpm/tpm.h

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel-next/+/51062105b0e7d2807c03b79bd99593847679626e

commit 51062105b0e7d2807c03b79bd99593847679626e
Author: Guenter Roeck <linux@roeck-us.net>
Date: Thu Nov 16 21:42:33 2017

BACKPORT: tpm: Add explicit chip->ops locking for sysfs attributes.

Add explicit chip->ops locking for all sysfs attributes.
This lets us support those attributes on tpm2 devices.

BUG= chromium:763394 
TEST=Run TPM test cases

Change-Id: I0be8c0e29d34b03896f28576b1b2a99a96304f33
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(am from https://patchwork.kernel.org/patch/10062133/)

[modify] https://crrev.com/51062105b0e7d2807c03b79bd99593847679626e/drivers/char/tpm/tpm-sysfs.c
[modify] https://crrev.com/51062105b0e7d2807c03b79bd99593847679626e/drivers/char/tpm/tpm-chip.c

Comment 7 by groeck@chromium.org, Nov 17 2017

Status: Started (was: Assigned)

Comment 8 by groeck@chromium.org, Nov 17 2017

Status: Fixed (was: Started)
Problem should be fixed in chromeos-4.14 final.

Comment 9 by groeck@chromium.org, Nov 17 2017

WontFix in chromeos-4.12.

Sign in to add a comment