LinuxLists.cc - [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> laptops I have:

...

> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> [ 17.868852] #PF: supervisor read access in kernel mode
> [ 17.868854] #PF: error_code(0x0000) - not-present page
> [ 17.868855] PGD 0 P4D 0
> [ 17.868858] Oops: 0000 [#1] SMP PTI
> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> [ 17.868879] Call Trace:
> [ 17.868884] seq_read+0x95/0x470
> [ 17.868887] ? security_file_permission+0x150/0x160
> [ 17.868889] vfs_read+0xaa/0x190
> [ 17.868891] ksys_read+0x67/0xe0
> [ 17.868893] __x64_sys_read+0x1a/0x20
> [ 17.868896] do_syscall_64+0x52/0xc0
> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> [ 17.868901] Code: Bad RIP value.
> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops ucsi_acpi roles cec
> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> [ 17.868951] CR2: 000000000000002c
> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0

It is possible but initially feels a bit weird:

- sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
+ sizeof(TCG_SPECID_SIG)) ||
+ !efispecid->num_algs || count != efispecid->num_algs) {

Assuming that check does not pass because of a more strict constraint,
__calc_tpm2_event_size() returns 0.

It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:

static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
struct tcg_pcr_event *event_header)
{
return __calc_tpm2_event_size(event, event_header, false);
}

I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
whenever this happens.

Are you 100% sure that it is exactly this commit that triggers the bug?

/Jarkko

2020-09-28 14:19:56

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

Hi Jarkko,

> On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
>
> On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
>> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
>> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
>> laptops I have:
>
> ...
>
>> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
>> [ 17.868852] #PF: supervisor read access in kernel mode
>> [ 17.868854] #PF: error_code(0x0000) - not-present page
>> [ 17.868855] PGD 0 P4D 0
>> [ 17.868858] Oops: 0000 [#1] SMP PTI
>> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
>> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
>> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
>> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
>> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
>> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
>> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
>> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
>> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
>> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
>> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
>> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
>> [ 17.868879] Call Trace:
>> [ 17.868884] seq_read+0x95/0x470
>> [ 17.868887] ? security_file_permission+0x150/0x160
>> [ 17.868889] vfs_read+0xaa/0x190
>> [ 17.868891] ksys_read+0x67/0xe0
>> [ 17.868893] __x64_sys_read+0x1a/0x20
>> [ 17.868896] do_syscall_64+0x52/0xc0
>> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 17.868900] RIP: 0033:0x7f9d83be91dc
>> [ 17.868901] Code: Bad RIP value.
>> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
>> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
>> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
>> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
>> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
>> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
>> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops ucsi_acpi ro
> les cec
>> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
>> [ 17.868951] CR2: 000000000000002c
>> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
>> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
>> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
>> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
>> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
>> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
>> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
>> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
>> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
>> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
>> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
>
> It is possible but initially feels a bit weird:
>
> - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> + sizeof(TCG_SPECID_SIG)) ||
> + !efispecid->num_algs || count != efispecid->num_algs) {
>
> Assuming that check does not pass because of a more strict constraint,
> __calc_tpm2_event_size() returns 0.
>
> It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
>
> static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> struct tcg_pcr_event *event_header)
> {
> return __calc_tpm2_event_size(event, event_header, false);
> }
>
> I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> whenever this happens.
>
> Are you 100% sure that it is exactly this commit that triggers the bug?

Yes I am 100% sure. The issue happens all the time, and never happens if I checkout the previous commit.

Kai-Heng

>
> /Jarkko

2020-09-28 16:19:41

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Mon, 28 Sep 2020 at 17:52, Jarkko Sakkinen
<[email protected]> wrote:
>
> On Mon, Sep 28, 2020 at 10:16:07PM +0800, Kai-Heng Feng wrote:
> > Hi Jarkko,
> >
> > > On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> > >> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> > >> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> > >> laptops I have:
> > >
> > > ...
> > >
> > >> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> > >> [ 17.868852] #PF: supervisor read access in kernel mode
> > >> [ 17.868854] #PF: error_code(0x0000) - not-present page
> > >> [ 17.868855] PGD 0 P4D 0
> > >> [ 17.868858] Oops: 0000 [#1] SMP PTI
> > >> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> > >> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> > >> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > >> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > >> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > >> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > >> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > >> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > >> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > >> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > >> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > >> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > >> [ 17.868879] Call Trace:
> > >> [ 17.868884] seq_read+0x95/0x470
> > >> [ 17.868887] ? security_file_permission+0x150/0x160
> > >> [ 17.868889] vfs_read+0xaa/0x190
> > >> [ 17.868891] ksys_read+0x67/0xe0
> > >> [ 17.868893] __x64_sys_read+0x1a/0x20
> > >> [ 17.868896] do_syscall_64+0x52/0xc0
> > >> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > >> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> > >> [ 17.868901] Code: Bad RIP value.
> > >> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> > >> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> > >> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> > >> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> > >> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> > >> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> > >> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops ucsi_acpi ro
> > > les cec
> > >> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> > >> [ 17.868951] CR2: 000000000000002c
> > >> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> > >> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > >> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > >> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > >> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > >> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > >> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > >> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > >> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > >> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > >> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > >
> > > It is possible but initially feels a bit weird:
> > >
> > > - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > > + sizeof(TCG_SPECID_SIG)) ||
> > > + !efispecid->num_algs || count != efispecid->num_algs) {
> > >
> > > Assuming that check does not pass because of a more strict constraint,
> > > __calc_tpm2_event_size() returns 0.
> > >
> > > It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
> > >
> > > static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> > > struct tcg_pcr_event *event_header)
> > > {
> > > return __calc_tpm2_event_size(event, event_header, false);
> > > }
> > >
> > > I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> > > tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> > > whenever this happens.
> > >
> > > Are you 100% sure that it is exactly this commit that triggers the bug?
> >
> > Yes I am 100% sure. The issue happens all the time, and never happens
> > if I checkout the previous commit.
>
> OK, had to just "sanity check". Thank you. Just sounds extremely weird
> because there are no new derefences in the path.
>
> Looking at the call sites in *_start:
>
> if (*pos > 0) {
> addr += size;
> event = addr;
> size = calc_tpm2_event_size(event, event_header);
> if ((addr + size >= limit) || (size == 0))
> return NULL;
> }
>
> for (i = 0; i < (*pos - 1); i++) {
> event = addr;
> size = calc_tpm2_event_size(event, event_header);
>
> if ((addr + size >= limit) || (size == 0))
> return NULL;
> addr += size;
> }
>
> we just essentially return NULL and efispecid is dereferenced two times
> instead of one. Especially, if this happens on your all machines it
> sounds weird.
>
> Kenneth [1] was experiencing a similar issue. Kenneth, can you try out
> if going one commit before 7f3d176f5f7e sorts out the issue?
>
> This sounds serious enough that I rather revert the commit if it is not
> rooted quickly but I added some people to the CC list who worked on this
> in the past. Perhaps they spot something I'm blind to ATM.
>

In tpm2_bios_measurements_start(), we dereference tpm_bios_log to
access bios_event_log and bios_event_log_end without checking tpm_bios
for NULL. This is where the crash seems to occur.

2020-09-28 16:31:06

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Mon, 28 Sep 2020 at 18:15, Ard Biesheuvel <[email protected]> wrote:
>
> On Mon, 28 Sep 2020 at 17:52, Jarkko Sakkinen
> <[email protected]> wrote:
> >
> > On Mon, Sep 28, 2020 at 10:16:07PM +0800, Kai-Heng Feng wrote:
> > > Hi Jarkko,
> > >
> > > > On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
> > > >
> > > > On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> > > >> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> > > >> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> > > >> laptops I have:
> > > >
> > > > ...
> > > >
> > > >> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> > > >> [ 17.868852] #PF: supervisor read access in kernel mode
> > > >> [ 17.868854] #PF: error_code(0x0000) - not-present page
> > > >> [ 17.868855] PGD 0 P4D 0
> > > >> [ 17.868858] Oops: 0000 [#1] SMP PTI
> > > >> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> > > >> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> > > >> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > > >> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > > >> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > > >> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > > >> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > > >> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > > >> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > > >> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > > >> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > > >> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > >> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > > >> [ 17.868879] Call Trace:
> > > >> [ 17.868884] seq_read+0x95/0x470
> > > >> [ 17.868887] ? security_file_permission+0x150/0x160
> > > >> [ 17.868889] vfs_read+0xaa/0x190
> > > >> [ 17.868891] ksys_read+0x67/0xe0
> > > >> [ 17.868893] __x64_sys_read+0x1a/0x20
> > > >> [ 17.868896] do_syscall_64+0x52/0xc0
> > > >> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > >> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> > > >> [ 17.868901] Code: Bad RIP value.
> > > >> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> > > >> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> > > >> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> > > >> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> > > >> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> > > >> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> > > >> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops ucsi_acpi ro
> > > > les cec
> > > >> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> > > >> [ 17.868951] CR2: 000000000000002c
> > > >> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> > > >> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > > >> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > > >> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > > >> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > > >> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > > >> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > > >> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > > >> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > > >> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > > >> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > >> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > > >
> > > > It is possible but initially feels a bit weird:
> > > >
> > > > - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > > > + sizeof(TCG_SPECID_SIG)) ||
> > > > + !efispecid->num_algs || count != efispecid->num_algs) {
> > > >
> > > > Assuming that check does not pass because of a more strict constraint,
> > > > __calc_tpm2_event_size() returns 0.
> > > >
> > > > It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
> > > >
> > > > static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> > > > struct tcg_pcr_event *event_header)
> > > > {
> > > > return __calc_tpm2_event_size(event, event_header, false);
> > > > }
> > > >
> > > > I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> > > > tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> > > > whenever this happens.
> > > >
> > > > Are you 100% sure that it is exactly this commit that triggers the bug?
> > >
> > > Yes I am 100% sure. The issue happens all the time, and never happens
> > > if I checkout the previous commit.
> >
> > OK, had to just "sanity check". Thank you. Just sounds extremely weird
> > because there are no new derefences in the path.
> >
> > Looking at the call sites in *_start:
> >
> > if (*pos > 0) {
> > addr += size;
> > event = addr;
> > size = calc_tpm2_event_size(event, event_header);
> > if ((addr + size >= limit) || (size == 0))
> > return NULL;
> > }
> >
> > for (i = 0; i < (*pos - 1); i++) {
> > event = addr;
> > size = calc_tpm2_event_size(event, event_header);
> >
> > if ((addr + size >= limit) || (size == 0))
> > return NULL;
> > addr += size;
> > }
> >
> > we just essentially return NULL and efispecid is dereferenced two times
> > instead of one. Especially, if this happens on your all machines it
> > sounds weird.
> >
> > Kenneth [1] was experiencing a similar issue. Kenneth, can you try out
> > if going one commit before 7f3d176f5f7e sorts out the issue?
> >
> > This sounds serious enough that I rather revert the commit if it is not
> > rooted quickly but I added some people to the CC list who worked on this
> > in the past. Perhaps they spot something I'm blind to ATM.
> >
>
> In tpm2_bios_measurements_start(), we dereference tpm_bios_log to
> access bios_event_log and bios_event_log_end without checking tpm_bios
> for NULL. This is where the crash seems to occur.

It's one level down from there:

18: 48 8b 47 70 mov 0x70(%rdi),%rax
1c: 4c 8b a0 d0 06 00 00 mov 0x6d0(%rax),%r12
23: 48 8b 88 d8 06 00 00 mov 0x6d8(%rax),%rcx
2a: 41 8b 5c 24 1c mov 0x1c(%r12),%ebx <-- trapping instruction

%rdi is is seq_file *m, so %rax is the tpm_chip pointer 'chip'.

The tpm_bios_log struct is embedded in the tpm_chip struct, but its
values are bogus.

So when we assign event_header to the value of
tpm_bios_log->bios_event_log and attempt to dereference it to get at
its event_size member, we crash. (0x1c is the offset of event_size in
struct tcg_pcr_event)

2020-09-28 17:14:52

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Mon, Sep 28, 2020 at 06:15:00PM +0200, Ard Biesheuvel wrote:
> > > > It is possible but initially feels a bit weird:
> > > >
> > > > - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > > > + sizeof(TCG_SPECID_SIG)) ||
> > > > + !efispecid->num_algs || count != efispecid->num_algs) {

^^^

> In tpm2_bios_measurements_start(), we dereference tpm_bios_log to
> access bios_event_log and bios_event_log_end without checking tpm_bios
> for NULL. This is where the crash seems to occur.

That's a good guess. Just a bit confused how that particular patch can
have the effect: it has two deferences to efispecid instead of one in
the same statement. Would be interesting to hear if the bug is triggered
in Kenneth's environment by the exact same commit.

/Jarkko

2020-09-28 18:26:24

by Kenneth Crudup

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Mon, 28 Sep 2020, Jarkko Sakkinen wrote:

> That's a good guess. Just a bit confused how that particular patch can
> have the effect: it has two deferences to efispecid instead of one in
> the same statement. Would be interesting to hear if the bug is triggered
> in Kenneth's environment by the exact same commit.

I can confirm that reverting 7f3d176f5f stops my kernel from panic()ing:

$ sudo fwupdtpmevlog
[sudo] password for kenny:
Failed to parse file: attempted to read 0x10 bytes from buffer of 0x00
$

It would normally OOPS at that point.

-Kenny

--
Kenneth R. Crudup Sr. SW Engineer, Scott County Consulting, Orange County CA

2020-09-29 17:55:47

by Mimi Zohar

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Mon, 2020-09-28 at 22:16 +0800, Kai-Heng Feng wrote:
> Hi Jarkko,
>
> > On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
> >
> > On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> >> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> >> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> >> laptops I have:
> >
> > ...
> >
> >> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> >> [ 17.868852] #PF: supervisor read access in kernel mode
> >> [ 17.868854] #PF: error_code(0x0000) - not-present page
> >> [ 17.868855] PGD 0 P4D 0
> >> [ 17.868858] Oops: 0000 [#1] SMP PTI
> >> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> >> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> >> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> >> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> >> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> >> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> >> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> >> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> >> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> >> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> >> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> >> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> >> [ 17.868879] Call Trace:
> >> [ 17.868884] seq_read+0x95/0x470
> >> [ 17.868887] ? security_file_permission+0x150/0x160
> >> [ 17.868889] vfs_read+0xaa/0x190
> >> [ 17.868891] ksys_read+0x67/0xe0
> >> [ 17.868893] __x64_sys_read+0x1a/0x20
> >> [ 17.868896] do_syscall_64+0x52/0xc0
> >> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> >> [ 17.868901] Code: Bad RIP value.
> >> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> >> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> >> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> >> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> >> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> >> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> >> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops u
cs
> i_acpi r
> o
> > les cec
> >> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> >> [ 17.868951] CR2: 000000000000002c
> >> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> >> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> >> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> >> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> >> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> >> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> >> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> >> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> >> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> >> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> >> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> >
> > It is possible but initially feels a bit weird:
> >
> > - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > + sizeof(TCG_SPECID_SIG)) ||
> > + !efispecid->num_algs || count != efispecid->num_algs) {
> >
> > Assuming that check does not pass because of a more strict constraint,
> > __calc_tpm2_event_size() returns 0.
> >
> > It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
> >
> > static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> > struct tcg_pcr_event *event_header)
> > {
> > return __calc_tpm2_event_size(event, event_header, false);
> > }
> >
> > I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> > tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> > whenever this happens.
> >
> > Are you 100% sure that it is exactly this commit that triggers the bug?
>
> Yes I am 100% sure. The issue happens all the time, and never happens
> if I checkout the previous commit.

I'm seeing this too on my test Ubuntu laptop. Reverting the patch
fixes it, but there's no data.

Mimi

2020-09-30 02:22:17

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Tue, Sep 29, 2020 at 01:52:04PM -0400, Mimi Zohar wrote:
> On Mon, 2020-09-28 at 22:16 +0800, Kai-Heng Feng wrote:
> > Hi Jarkko,
> >
> > > On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> > >> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> > >> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> > >> laptops I have:
> > >
> > > ...
> > >
> > >> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> > >> [ 17.868852] #PF: supervisor read access in kernel mode
> > >> [ 17.868854] #PF: error_code(0x0000) - not-present page
> > >> [ 17.868855] PGD 0 P4D 0
> > >> [ 17.868858] Oops: 0000 [#1] SMP PTI
> > >> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> > >> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> > >> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > >> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > >> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > >> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > >> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > >> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > >> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > >> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > >> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > >> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > >> [ 17.868879] Call Trace:
> > >> [ 17.868884] seq_read+0x95/0x470
> > >> [ 17.868887] ? security_file_permission+0x150/0x160
> > >> [ 17.868889] vfs_read+0xaa/0x190
> > >> [ 17.868891] ksys_read+0x67/0xe0
> > >> [ 17.868893] __x64_sys_read+0x1a/0x20
> > >> [ 17.868896] do_syscall_64+0x52/0xc0
> > >> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > >> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> > >> [ 17.868901] Code: Bad RIP value.
> > >> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> > >> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> > >> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> > >> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> > >> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> > >> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> > >> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops u
> cs
> > i_acpi r
> > o
> > > les cec
> > >> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> > >> [ 17.868951] CR2: 000000000000002c
> > >> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> > >> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > >> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > >> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > >> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > >> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > >> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > >> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > >> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > >> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > >> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > >
> > > It is possible but initially feels a bit weird:
> > >
> > > - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > > + sizeof(TCG_SPECID_SIG)) ||
> > > + !efispecid->num_algs || count != efispecid->num_algs) {
> > >
> > > Assuming that check does not pass because of a more strict constraint,
> > > __calc_tpm2_event_size() returns 0.
> > >
> > > It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
> > >
> > > static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> > > struct tcg_pcr_event *event_header)
> > > {
> > > return __calc_tpm2_event_size(event, event_header, false);
> > > }
> > >
> > > I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> > > tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> > > whenever this happens.
> > >
> > > Are you 100% sure that it is exactly this commit that triggers the bug?
> >
> > Yes I am 100% sure. The issue happens all the time, and never happens
> > if I checkout the previous commit.
>
> I'm seeing this too on my test Ubuntu laptop. Reverting the patch
> fixes it, but there's no data.

I do not have yet any reasonable answer to this and my v5.10 PR is
running late. Does everyone agree that I should revert the patch?

>
> Mimi

/Jarkko

2020-10-08 09:19:00

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

> On Sep 30, 2020, at 10:20, Jarkko Sakkinen <[email protected]> wrote:
>
> On Tue, Sep 29, 2020 at 01:52:04PM -0400, Mimi Zohar wrote:
>> On Mon, 2020-09-28 at 22:16 +0800, Kai-Heng Feng wrote:
>>> Hi Jarkko,
>>>
>>>> On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
>>>>
>>>> On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
>>>>> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
>>>>> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
>>>>> laptops I have:
>>>>
>>>> ...
>>>>
>>>>> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
>>>>> [ 17.868852] #PF: supervisor read access in kernel mode
>>>>> [ 17.868854] #PF: error_code(0x0000) - not-present page
>>>>> [ 17.868855] PGD 0 P4D 0
>>>>> [ 17.868858] Oops: 0000 [#1] SMP PTI
>>>>> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
>>>>> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
>>>>> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
>>>>> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
>>>>> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
>>>>> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
>>>>> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
>>>>> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
>>>>> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
>>>>> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
>>>>> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
>>>>> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
>>>>> [ 17.868879] Call Trace:
>>>>> [ 17.868884] seq_read+0x95/0x470
>>>>> [ 17.868887] ? security_file_permission+0x150/0x160
>>>>> [ 17.868889] vfs_read+0xaa/0x190
>>>>> [ 17.868891] ksys_read+0x67/0xe0
>>>>> [ 17.868893] __x64_sys_read+0x1a/0x20
>>>>> [ 17.868896] do_syscall_64+0x52/0xc0
>>>>> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>>> [ 17.868900] RIP: 0033:0x7f9d83be91dc
>>>>> [ 17.868901] Code: Bad RIP value.
>>>>> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
>>>>> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
>>>>> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
>>>>> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
>>>>> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
>>>>> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
>>>>> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops u
>> cs
>>> i_acpi r
>>> o
>>>> les cec
>>>>> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
>>>>> [ 17.868951] CR2: 000000000000002c
>>>>> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
>>>>> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
>>>>> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
>>>>> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
>>>>> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
>>>>> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
>>>>> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
>>>>> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
>>>>> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
>>>>> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
>>>>> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
>>>>
>>>> It is possible but initially feels a bit weird:
>>>>
>>>> - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
>>>> + sizeof(TCG_SPECID_SIG)) ||
>>>> + !efispecid->num_algs || count != efispecid->num_algs) {
>>>>
>>>> Assuming that check does not pass because of a more strict constraint,
>>>> __calc_tpm2_event_size() returns 0.
>>>>
>>>> It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
>>>>
>>>> static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
>>>> struct tcg_pcr_event *event_header)
>>>> {
>>>> return __calc_tpm2_event_size(event, event_header, false);
>>>> }
>>>>
>>>> I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
>>>> tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
>>>> whenever this happens.
>>>>
>>>> Are you 100% sure that it is exactly this commit that triggers the bug?
>>>
>>> Yes I am 100% sure. The issue happens all the time, and never happens
>>> if I checkout the previous commit.
>>
>> I'm seeing this too on my test Ubuntu laptop. Reverting the patch
>> fixes it, but there's no data.
>
> I do not have yet any reasonable answer to this and my v5.10 PR is
> running late. Does everyone agree that I should revert the patch?

Given that there are multiple users confirmed reverting the commit helps, can you please revert it and Cc: linux-stable?

Thanks!

Kai-Heng

>
>>
>> Mimi
>
> /Jarkko

2020-10-09 16:12:06

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Thu, Oct 08, 2020 at 05:09:06PM +0800, Kai-Heng Feng wrote:
> > I do not have yet any reasonable answer to this and my v5.10 PR is
> > running late. Does everyone agree that I should revert the patch?
>
> Given that there are multiple users confirmed reverting the commit
> helps, can you please revert it and Cc: linux-stable?

I already sent the PR, but I schedule the revert to my rc2 PR.

> Thanks!
>
> Kai-Heng

/Jarkko

2020-10-13 17:15:39

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Tue, Oct 13, 2020 at 09:31:39AM -0500, Tyler Hicks wrote:
> Sorry for coming in so late, I've been on an extended vacation with
> little connectivity.
>
> On 2020-10-09 19:06:54, Jarkko Sakkinen wrote:
> > On Thu, Oct 08, 2020 at 05:09:06PM +0800, Kai-Heng Feng wrote:
> > > > I do not have yet any reasonable answer to this and my v5.10 PR is
> > > > running late. Does everyone agree that I should revert the patch?
> > >
> > > Given that there are multiple users confirmed reverting the commit
> > > helps, can you please revert it and Cc: linux-stable?
> >
> > I already sent the PR, but I schedule the revert to my rc2 PR.
>
> I'll try to better understand what's going on. I, too, am confused about
> how the change would introduce the reported regression. I've only
> skimmed the thread so far but it feels like there's possibly a latent
> issue that the change may be uncovering on certain systems.
>
> FWIW, we've had this patch applied to our internal kernel for a month
> and haven't seen any issues.
>
> Tyler

Tyler, it will be a while before even rc1 is out, i.e. there's window
for investigation.

/Jarkko

2020-10-13 23:54:07

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

Sorry for coming in so late, I've been on an extended vacation with
little connectivity.

On 2020-10-09 19:06:54, Jarkko Sakkinen wrote:
> On Thu, Oct 08, 2020 at 05:09:06PM +0800, Kai-Heng Feng wrote:
> > > I do not have yet any reasonable answer to this and my v5.10 PR is
> > > running late. Does everyone agree that I should revert the patch?
> >
> > Given that there are multiple users confirmed reverting the commit
> > helps, can you please revert it and Cc: linux-stable?
>
> I already sent the PR, but I schedule the revert to my rc2 PR.

I'll try to better understand what's going on. I, too, am confused about
how the change would introduce the reported regression. I've only
skimmed the thread so far but it feels like there's possibly a latent
issue that the change may be uncovering on certain systems.

FWIW, we've had this patch applied to our internal kernel for a month
and haven't seen any issues.

Tyler

>
> > Thanks!
> >
> > Kai-Heng
>
> /Jarkko
>

2020-10-21 10:13:45

by Mimi Zohar

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On Tue, 2020-09-29 at 13:52 -0400, Mimi Zohar wrote:
> On Mon, 2020-09-28 at 22:16 +0800, Kai-Heng Feng wrote:
> > Hi Jarkko,
> >
> > > On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> > >> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> > >> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> > >> laptops I have:
> > >
> > > ...
> > >
> > >> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> > >> [ 17.868852] #PF: supervisor read access in kernel mode
> > >> [ 17.868854] #PF: error_code(0x0000) - not-present page
> > >> [ 17.868855] PGD 0 P4D 0
> > >> [ 17.868858] Oops: 0000 [#1] SMP PTI
> > >> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> > >> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> > >> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > >> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > >> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > >> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > >> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > >> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > >> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > >> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > >> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > >> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > >> [ 17.868879] Call Trace:
> > >> [ 17.868884] seq_read+0x95/0x470
> > >> [ 17.868887] ? security_file_permission+0x150/0x160
> > >> [ 17.868889] vfs_read+0xaa/0x190
> > >> [ 17.868891] ksys_read+0x67/0xe0
> > >> [ 17.868893] __x64_sys_read+0x1a/0x20
> > >> [ 17.868896] do_syscall_64+0x52/0xc0
> > >> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > >> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> > >> [ 17.868901] Code: Bad RIP value.
> > >> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> > >> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> > >> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> > >> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> > >> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> > >> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> > >> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops
u
> cs
> > i_acpi r
> > o
> > > les cec
> > >> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> > >> [ 17.868951] CR2: 000000000000002c
> > >> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> > >> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > >> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > >> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > >> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > >> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > >> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > >> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > >> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > >> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > >> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > >
> > > It is possible but initially feels a bit weird:
> > >
> > > - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > > + sizeof(TCG_SPECID_SIG)) ||
> > > + !efispecid->num_algs || count != efispecid->num_algs) {
> > >
> > > Assuming that check does not pass because of a more strict constraint,
> > > __calc_tpm2_event_size() returns 0.
> > >
> > > It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
> > >
> > > static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> > > struct tcg_pcr_event *event_header)
> > > {
> > > return __calc_tpm2_event_size(event, event_header, false);
> > > }
> > >
> > > I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> > > tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> > > whenever this happens.
> > >
> > > Are you 100% sure that it is exactly this commit that triggers the bug?
> >
> > Yes I am 100% sure. The issue happens all the time, and never happens
> > if I checkout the previous commit.
>
> I'm seeing this too on my test Ubuntu laptop. Reverting the patch
> fixes it, but there's no data.

For a while I wasn't able to boot the test system with secure boot
enabled. The NULL pointer dereference problem occurred during the
period of time that secure boot was disabled. Once the secure boot
issue was resolved, the NULL pointer dereference problem was resolved
as well, when booting with/without secure boot enabled.

Mimi

2020-10-21 12:39:22

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On 2020-10-20 17:07:50, Mimi Zohar wrote:
> On Tue, 2020-09-29 at 13:52 -0400, Mimi Zohar wrote:
> > On Mon, 2020-09-28 at 22:16 +0800, Kai-Heng Feng wrote:
> > > Hi Jarkko,
> > >
> > > > On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
> > > >
> > > > On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> > > >> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> > > >> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> > > >> laptops I have:
> > > >
> > > > ...
> > > >
> > > >> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> > > >> [ 17.868852] #PF: supervisor read access in kernel mode
> > > >> [ 17.868854] #PF: error_code(0x0000) - not-present page
> > > >> [ 17.868855] PGD 0 P4D 0
> > > >> [ 17.868858] Oops: 0000 [#1] SMP PTI
> > > >> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> > > >> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> > > >> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > > >> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > > >> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > > >> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > > >> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > > >> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > > >> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > > >> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > > >> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > > >> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > >> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > > >> [ 17.868879] Call Trace:
> > > >> [ 17.868884] seq_read+0x95/0x470
> > > >> [ 17.868887] ? security_file_permission+0x150/0x160
> > > >> [ 17.868889] vfs_read+0xaa/0x190
> > > >> [ 17.868891] ksys_read+0x67/0xe0
> > > >> [ 17.868893] __x64_sys_read+0x1a/0x20
> > > >> [ 17.868896] do_syscall_64+0x52/0xc0
> > > >> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > >> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> > > >> [ 17.868901] Code: Bad RIP value.
> > > >> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> > > >> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> > > >> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> > > >> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> > > >> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> > > >> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> > > >> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops
> u
> > cs
> > > i_acpi r
> > > o
> > > > les cec
> > > >> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> > > >> [ 17.868951] CR2: 000000000000002c
> > > >> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> > > >> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> > > >> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> > > >> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> > > >> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> > > >> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> > > >> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> > > >> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> > > >> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> > > >> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> > > >> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > >> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> > > >
> > > > It is possible but initially feels a bit weird:
> > > >
> > > > - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > > > + sizeof(TCG_SPECID_SIG)) ||
> > > > + !efispecid->num_algs || count != efispecid->num_algs) {
> > > >
> > > > Assuming that check does not pass because of a more strict constraint,
> > > > __calc_tpm2_event_size() returns 0.
> > > >
> > > > It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
> > > >
> > > > static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> > > > struct tcg_pcr_event *event_header)
> > > > {
> > > > return __calc_tpm2_event_size(event, event_header, false);
> > > > }
> > > >
> > > > I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> > > > tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> > > > whenever this happens.
> > > >
> > > > Are you 100% sure that it is exactly this commit that triggers the bug?
> > >
> > > Yes I am 100% sure. The issue happens all the time, and never happens
> > > if I checkout the previous commit.
> >
> > I'm seeing this too on my test Ubuntu laptop. Reverting the patch
> > fixes it, but there's no data.
>
> For a while I wasn't able to boot the test system with secure boot
> enabled. The NULL pointer dereference problem occurred during the
> period of time that secure boot was disabled. Once the secure boot
> issue was resolved, the NULL pointer dereference problem was resolved
> as well, when booting with/without secure boot enabled.

I noticed that Kai had disabled secure boot, as well. I found a Thinkpad
with the same BIOS revision as Kai's Thinkpad (N27ET38W - 1.24) and
tested an unpatched v5.9 kernel running on an up-to-date Ubuntu 20.04.1
userspace. The main difference is that he has a Thinkpad T580 and I was
using a Thinkpad P52s. I was unable to reproduce the crash either by
running fwupdtpmevlog or cat'ing /sys/kernel/security/tpm0/binary_bios_measurements.

I noticed that Kai had cleared his secure boot allowed signature
database (db) so I also tried that but was still unsuccessful in
reproducing the crash.

Of course, I also tested with secure boot enabled and also after
toggling every secure boot and TPM related option in the BIOS. Still no
luck.

Kenneth mentioned elsewhere[1] that he experienced the crash with
Ubuntu's 5.8.0-18 kernel so I also tried that but still no luck.

Everyone that has reported this crash[1][2][3][4] seems to have been
running with an Ubuntu userspace. I do see that Ubuntu recently fixed a
shim bug[5] on Sept 24. Ubuntu auto-installs security updates by default
but this was not a security update so perhaps folks that were initially
affected by this crash hadn't yet applied the update. It looks unrelated
to me but it might explain why I can't reproduce this crash now, how the
crash no longer affects Mimi, and why nobody else is complaining about
the crash in recent weeks.

Kai, could you please try once more with v5.9 and ensuring that your
Ubuntu system is fully updated? If you're able to reproduce it with
v5.9, please send me your kernel config and your raw
/sys/kernel/security/tpm0/binary_bios_measurements file contents after
rebooting into a working kernel. I really appreciate it!

Tyler

[1] https://lore.kernel.org/linux-integrity/alpine.DEB.2.23.453.2009121522400.7398@xps-7390/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
[4] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1898998
[5] https://bugs.launchpad.net/ubuntu/+source/shim/+bug/1864223

>
> Mimi
>

2020-10-26 06:57:13

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

>
> Tyler
>
> [1] https://lore.kernel.org/linux-integrity/alpine.DEB.2.23.453.2009121522400.7398@xps-7390/
> [2] https://lore.kernel.org/lkml/[email protected]/
> [3] https://lore.kernel.org/lkml/[email protected]/
> [4] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1898998
> [5] https://bugs.launchpad.net/ubuntu/+source/shim/+bug/1864223
>
>>
>> Mimi

Attachments:

config-5.10.0-rc1 (180.44 kB)
dmesg (85.54 kB)
Download all attachments

2020-10-27 14:06:22

[permalink] [raw]

Subject: Re: [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference

On 2020-10-26 13:49:59, Kai-Heng Feng wrote:
>
>
> > On Oct 21, 2020, at 13:48, Tyler Hicks <[email protected]> wrote:
> >
> > On 2020-10-20 17:07:50, Mimi Zohar wrote:
> >> On Tue, 2020-09-29 at 13:52 -0400, Mimi Zohar wrote:
> >>> On Mon, 2020-09-28 at 22:16 +0800, Kai-Heng Feng wrote:
> >>>> Hi Jarkko,
> >>>>
> >>>>> On Sep 28, 2020, at 22:06, Jarkko Sakkinen <[email protected]> wrote:
> >>>>>
> >>>>> On Mon, Sep 28, 2020 at 08:31:04PM +0800, Kai-Heng Feng wrote:
> >>>>>> Commit 7f3d176f5f7e "tpm: Require that all digests are present in
> >>>>>> TCG_PCR_EVENT2 structures" causes a null pointer dereference on all
> >>>>>> laptops I have:
> >>>>>
> >>>>> ...
> >>>>>
> >>>>>> [ 17.868849] BUG: kernel NULL pointer dereference, address: 000000000000002c
> >>>>>> [ 17.868852] #PF: supervisor read access in kernel mode
> >>>>>> [ 17.868854] #PF: error_code(0x0000) - not-present page
> >>>>>> [ 17.868855] PGD 0 P4D 0
> >>>>>> [ 17.868858] Oops: 0000 [#1] SMP PTI
> >>>>>> [ 17.868860] CPU: 0 PID: 1873 Comm: fwupd Not tainted 5.8.0-rc6+ #25
> >>>>>> [ 17.868861] Hardware name: LENOVO 20LAZ3TXCN/20LAZ3TXCN, BIOS N27ET38W (1.24 ) 11/28/2019
> >>>>>> [ 17.868866] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> >>>>>> [ 17.868868] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> >>>>>> [ 17.868869] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> >>>>>> [ 17.868871] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> >>>>>> [ 17.868872] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> >>>>>> [ 17.868873] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> >>>>>> [ 17.868874] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> >>>>>> [ 17.868875] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> >>>>>> [ 17.868876] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> >>>>>> [ 17.868877] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>>>> [ 17.868878] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> >>>>>> [ 17.868879] Call Trace:
> >>>>>> [ 17.868884] seq_read+0x95/0x470
> >>>>>> [ 17.868887] ? security_file_permission+0x150/0x160
> >>>>>> [ 17.868889] vfs_read+0xaa/0x190
> >>>>>> [ 17.868891] ksys_read+0x67/0xe0
> >>>>>> [ 17.868893] __x64_sys_read+0x1a/0x20
> >>>>>> [ 17.868896] do_syscall_64+0x52/0xc0
> >>>>>> [ 17.868898] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >>>>>> [ 17.868900] RIP: 0033:0x7f9d83be91dc
> >>>>>> [ 17.868901] Code: Bad RIP value.
> >>>>>> [ 17.868902] RSP: 002b:00007fff7f5e0250 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> >>>>>> [ 17.868903] RAX: ffffffffffffffda RBX: 00005651d262f420 RCX: 00007f9d83be91dc
> >>>>>> [ 17.868904] RDX: 0000000000001000 RSI: 00007fff7f5e0350 RDI: 0000000000000010
> >>>>>> [ 17.868905] RBP: 00007f9d83cc54a0 R08: 0000000000000000 R09: 00005651d26c1830
> >>>>>> [ 17.868906] R10: 00005651d2582010 R11: 0000000000000246 R12: 0000000000001000
> >>>>>> [ 17.868907] R13: 00007fff7f5e0350 R14: 0000000000000d68 R15: 00007f9d83cc48a0
> >>>>>> [ 17.868909] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic bnep joydev mei_hdcp wmi_bmof intel_rapl_msr intel_wmi_thunderbolt x86_pkg_temp_thermal intel_powerclamp coretemp nls_iso8859_1 kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd rapl input_leds intel_cstate snd_hda_intel snd_intel_dspcfg rmi_smbus iwlmvm snd_hda_codec serio_raw snd_hwdep mac80211 rmi_core snd_hda_core libarc4 uvcvideo snd_pcm videobuf2_vmalloc btusb videobuf2_memops iwlwifi videobuf2_v4l2 btrtl btbcm videobuf2_common btintel thunderbolt i915 bluetooth mei_me videodev thinkpad_acpi nvram cfg80211 ledtrig_audio mei mc ecdh_generic ecc i2c_algo_bit processor_thermal_device snd_seq_midi drm_kms_helper snd_seq_midi_event intel_soc_dts_iosf syscopyarea sysfillrect snd_rawmidi intel_pch_thermal sysimgblt intel_rapl_common intel_xhci_usb_role_switch fb_sys_fops
> >> u
> >>> cs
> >>>> i_acpi r
> >>>> o
> >>>>> les cec
> >>>>>> [ 17.868935] typec_ucsi typec nxp_nci_i2c snd_seq nxp_nci wmi nci nfc snd_timer snd_seq_device snd int3403_thermal soundcore int340x_thermal_zone video mac_hid int3400_thermal acpi_pad acpi_thermal_rel sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 btrfs blake2b_generic libcrc32c xor zstd_compress raid6_pq uas usb_storage psmouse e1000e nvme i2c_i801 i2c_smbus nvme_core intel_lpss_pci intel_lpss idma64 virt_dma pinctrl_sunrisepoint pinctrl_intel
> >>>>>> [ 17.868951] CR2: 000000000000002c
> >>>>>> [ 17.868953] ---[ end trace ee7716fff5dec2fb ]---
> >>>>>> [ 17.868955] RIP: 0010:tpm2_bios_measurements_start+0x38/0x1f0
> >>>>>> [ 17.868957] Code: 55 41 54 53 48 83 ec 30 4c 8b 16 65 48 8b 04 25 28 00 00 00 48 89 45 d0 48 8b 47 70 4c 8b a0 d0 06 00 00 48 8b 88 d8 06 00 00 <41> 8b 5c 24 1c 48 89 4d b0 48 89 d8 48 83 c3 20 4d 85 d2 75 31 4c
> >>>>>> [ 17.868958] RSP: 0018:ffff9da500a9fde0 EFLAGS: 00010282
> >>>>>> [ 17.868959] RAX: ffff917d03dc4000 RBX: 0000000000000000 RCX: 0000000000000010
> >>>>>> [ 17.868960] RDX: 0000000000001000 RSI: ffff917c99b19460 RDI: ffff917c99b19438
> >>>>>> [ 17.868961] RBP: ffff9da500a9fe38 R08: ffffbda4ffa33fc0 R09: ffff917cbfeae4c0
> >>>>>> [ 17.868962] R10: 0000000000000000 R11: 0000000000000002 R12: 0000000000000010
> >>>>>> [ 17.868963] R13: ffff917c99b19438 R14: ffff917c99b19460 R15: ffff917c99b19470
> >>>>>> [ 17.868964] FS: 00007f9d80988b00(0000) GS:ffff917d07400000(0000) knlGS:0000000000000000
> >>>>>> [ 17.868965] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>>>> [ 17.868966] CR2: 000000000000002c CR3: 0000000219b12004 CR4: 00000000003606f0
> >>>>>
> >>>>> It is possible but initially feels a bit weird:
> >>>>>
> >>>>> - sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> >>>>> + sizeof(TCG_SPECID_SIG)) ||
> >>>>> + !efispecid->num_algs || count != efispecid->num_algs) {
> >>>>>
> >>>>> Assuming that check does not pass because of a more strict constraint,
> >>>>> __calc_tpm2_event_size() returns 0.
> >>>>>
> >>>>> It is wrapped like this in drivers/char/tpm/eventlog/tpm2.c:
> >>>>>
> >>>>> static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> >>>>> struct tcg_pcr_event *event_header)
> >>>>> {
> >>>>> return __calc_tpm2_event_size(event, event_header, false);
> >>>>> }
> >>>>>
> >>>>> I.e. TPM_MEMUNMAP will not get executed because "do_mapping == false".
> >>>>> tpm2_bios_measurements_start() checks for "size == 0" and returns NULL
> >>>>> whenever this happens.
> >>>>>
> >>>>> Are you 100% sure that it is exactly this commit that triggers the bug?
> >>>>
> >>>> Yes I am 100% sure. The issue happens all the time, and never happens
> >>>> if I checkout the previous commit.
> >>>
> >>> I'm seeing this too on my test Ubuntu laptop. Reverting the patch
> >>> fixes it, but there's no data.
> >>
> >> For a while I wasn't able to boot the test system with secure boot
> >> enabled. The NULL pointer dereference problem occurred during the
> >> period of time that secure boot was disabled. Once the secure boot
> >> issue was resolved, the NULL pointer dereference problem was resolved
> >> as well, when booting with/without secure boot enabled.
> >
> > I noticed that Kai had disabled secure boot, as well. I found a Thinkpad
> > with the same BIOS revision as Kai's Thinkpad (N27ET38W - 1.24) and
> > tested an unpatched v5.9 kernel running on an up-to-date Ubuntu 20.04.1
> > userspace. The main difference is that he has a Thinkpad T580 and I was
> > using a Thinkpad P52s. I was unable to reproduce the crash either by
> > running fwupdtpmevlog or cat'ing /sys/kernel/security/tpm0/binary_bios_measurements.
> >
> > I noticed that Kai had cleared his secure boot allowed signature
> > database (db) so I also tried that but was still unsuccessful in
> > reproducing the crash.
> >
> > Of course, I also tested with secure boot enabled and also after
> > toggling every secure boot and TPM related option in the BIOS. Still no
> > luck.
> >
> > Kenneth mentioned elsewhere[1] that he experienced the crash with
> > Ubuntu's 5.8.0-18 kernel so I also tried that but still no luck.
> >
> > Everyone that has reported this crash[1][2][3][4] seems to have been
> > running with an Ubuntu userspace. I do see that Ubuntu recently fixed a
> > shim bug[5] on Sept 24. Ubuntu auto-installs security updates by default
> > but this was not a security update so perhaps folks that were initially
> > affected by this crash hadn't yet applied the update. It looks unrelated
> > to me but it might explain why I can't reproduce this crash now, how the
> > crash no longer affects Mimi, and why nobody else is complaining about
> > the crash in recent weeks.
> >
> > Kai, could you please try once more with v5.9 and ensuring that your
> > Ubuntu system is fully updated?
>
> Yes it's fully updated.
>
> > If you're able to reproduce it with
> > v5.9, please send me your kernel config and your raw
>
> Ok, please refer to attachment.
>
> > /sys/kernel/security/tpm0/binary_bios_measurements file contents after
> > rebooting into a working kernel. I really appreciate it!
>
> Hmm, it's empty.

Thanks, this was the key detail I was missing. I'm now able to reproduce
this crash locally by forcing an empty events log in the EFI stub.

I've got a patch that fixes this crash but I'm still reading TCG specs
to understand exactly why my change started triggering this and if
there's a second fix needed. I hope to have the patch(es) out tomorrow.

Tyler

>
> Kai-Heng
>

>
> >
> > Tyler
> >
> > [1] https://lore.kernel.org/linux-integrity/alpine.DEB.2.23.453.2009121522400.7398@xps-7390/
> > [2] https://lore.kernel.org/lkml/[email protected]/
> > [3] https://lore.kernel.org/lkml/[email protected]/
> > [4] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1898998
> > [5] https://bugs.launchpad.net/ubuntu/+source/shim/+bug/1864223
> >
> >>
> >> Mimi
>

2020-10-28 22:28:59

[permalink] [raw]

Subject: [PATCH] tpm: efi: Don't create binary_bios_measurements file for an empty log

Mimic the pre-existing ACPI and Device Tree event log behavior by not
creating the binary_bios_measurements file when the EFI TPM event log is
empty.

This fixes the following NULL pointer dereference that can occur when
reading /sys/kernel/security/tpm0/binary_bios_measurements after the
kernel received an empty event log from the firmware:

BUG: kernel NULL pointer dereference, address: 000000000000002c
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 2 PID: 3932 Comm: fwupdtpmevlog Not tainted 5.9.0-00003-g629990edad62 #17
Hardware name: LENOVO 20LCS03L00/20LCS03L00, BIOS N27ET38W (1.24 ) 11/28/2019
RIP: 0010:tpm2_bios_measurements_start+0x3a/0x550
Code: 54 53 48 83 ec 68 48 8b 57 70 48 8b 1e 65 48 8b 04 25 28 00 00 00 48 89 45 d0 31 c0 48 8b 82 c0 06 00 00 48 8b 8a c8 06 00 00 <44> 8b 60 1c 48 89 4d a0 4c 89 e2 49 83 c4 20 48 83 fb 00 75 2a 49
RSP: 0018:ffffa9c901203db0 EFLAGS: 00010246
RAX: 0000000000000010 RBX: 0000000000000000 RCX: 0000000000000010
RDX: ffff8ba1eb99c000 RSI: ffff8ba1e4ce8280 RDI: ffff8ba1e4ce8258
RBP: ffffa9c901203e40 R08: ffffa9c901203dd8 R09: ffff8ba1ec443300
R10: ffffa9c901203e50 R11: 0000000000000000 R12: ffff8ba1e4ce8280
R13: ffffa9c901203ef0 R14: ffffa9c901203ef0 R15: ffff8ba1e4ce8258
FS: 00007f6595460880(0000) GS:ffff8ba1ef880000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000000002c CR3: 00000007d8d18003 CR4: 00000000003706e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
? __kmalloc_node+0x113/0x320
? kvmalloc_node+0x31/0x80
seq_read+0x94/0x420
vfs_read+0xa7/0x190
ksys_read+0xa7/0xe0
__x64_sys_read+0x1a/0x20
do_syscall_64+0x37/0x80
entry_SYSCALL_64_after_hwframe+0x44/0xa9

In this situation, the bios_event_log pointer in the tpm_bios_log struct
was not NULL but was equal to the ZERO_SIZE_PTR (0x10) value. This was
due to the following kmemdup() in tpm_read_log_efi():

int tpm_read_log_efi(struct tpm_chip *chip)
{
...
/* malloc EventLog space */
log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
if (!log->bios_event_log) {
ret = -ENOMEM;
goto out;
}
...
}

When log_size is zero, due to an empty event log from firmware,
ZERO_SIZE_PTR is returned from kmemdup(). Upon a read of the
binary_bios_measurements file, the tpm2_bios_measurements_start()
function does not perform a ZERO_OR_NULL_PTR() check on the
bios_event_log pointer before dereferencing it.

Rather than add a ZERO_OR_NULL_PTR() check in functions that make use of
the bios_event_log pointer, simply avoid creating the
binary_bios_measurements_file as is done in other event log retrieval
backends.

Explicitly ignore all of the events in the final event log when the main
event log is empty. The list of events in the final event log cannot be
accurately parsed without referring to the first event in the main event
log (the event log header) so the final event log is useless in such a
situation.

Fixes: 58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table")
Link: https://lore.kernel.org/linux-integrity/[email protected]/
Reported-by: Kai-Heng Feng <[email protected]>
Reported-by: Kenneth R. Crudup <[email protected]>
Reported-by: Mimi Zohar <[email protected]>
Cc: Thiébaud Weksteen <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Signed-off-by: Tyler Hicks <[email protected]>
---
drivers/char/tpm/eventlog/efi.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 6bb023de17f1..35229e5143ca 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -41,6 +41,11 @@ int tpm_read_log_efi(struct tpm_chip *chip)
log_size = log_tbl->size;
memunmap(log_tbl);

+ if (!log_size) {
+ pr_warn("UEFI TPM log area empty\n");
+ return -EIO;
+ }
+
log_tbl = memremap(efi.tpm_log, sizeof(*log_tbl) + log_size,
MEMREMAP_WB);
if (!log_tbl) {

base-commit: ed8780e3f2ecc82645342d070c6b4e530532e680
--
2.25.1

2020-10-29 01:23:23

[permalink] [raw]

Subject: Re: [PATCH] tpm: efi: Don't create binary_bios_measurements file for an empty log

On 2020-10-28 11:30:11, Tyler Hicks wrote:
> So, we need help from Kai, Kenneth, or Mimi to verify my assumption that
> their firmware is providing an empty main event log and a populated
> final event log.

Hi Kai, Kenneth, and Mimi - could one or two of you please follow these
steps:

1) Apply the proposed patch in the grandparent of this email so that
your kernel doesn't crash
2) Revert commit 7f3d176f5f7e ("tpm: Require that all digests are
present in TCG_PCR_EVENT2 structures") so that
__calc_tpm2_event_size() goes back to being less picky and may treat
a final log event as a valid event log header
3) Add some debugging warnings in efi_tpm_eventlog_init() to check for
an empty main event log and a populated final event log, as shown
below
4) Boot the resulting kernel build, look for the warnings, and report
back

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index c1955d320fec..c4d2dbd5ed42 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -78,6 +78,9 @@ int __init efi_tpm_eventlog_init(void)
goto out;
}

+ WARN(!log_tbl->size && final_tbl->nr_events,
+ "nr_events = %llu\n", final_tbl->nr_events);
+
tbl_size = 0;
if (final_tbl->nr_events != 0) {
void *events = (void *)efi.tpm_final_log
@@ -95,6 +98,8 @@ int __init efi_tpm_eventlog_init(void)
goto out_calc;
}

+ WARN(!log_tbl->size && tbl_size, "tbl_size = %d\n", tbl_size);
+
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;

For your convenience, I've created a branch with these changes on top of
v5.9:

https://git.kernel.org/pub/scm/linux/kernel/git/tyhicks/linux.git/log/?h=tpm/bin-bios-measurements-debug

Thank you!

Tyler

2020-10-29 01:23:25

[permalink] [raw]

Subject: Re: [PATCH] tpm: efi: Don't create binary_bios_measurements file for an empty log

On 2020-10-28 10:41:02, Tyler Hicks wrote:
> Mimic the pre-existing ACPI and Device Tree event log behavior by not
> creating the binary_bios_measurements file when the EFI TPM event log is
> empty.
>
> This fixes the following NULL pointer dereference that can occur when
> reading /sys/kernel/security/tpm0/binary_bios_measurements after the
> kernel received an empty event log from the firmware:
>
> BUG: kernel NULL pointer dereference, address: 000000000000002c
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 2 PID: 3932 Comm: fwupdtpmevlog Not tainted 5.9.0-00003-g629990edad62 #17
> Hardware name: LENOVO 20LCS03L00/20LCS03L00, BIOS N27ET38W (1.24 ) 11/28/2019
> RIP: 0010:tpm2_bios_measurements_start+0x3a/0x550
> Code: 54 53 48 83 ec 68 48 8b 57 70 48 8b 1e 65 48 8b 04 25 28 00 00 00 48 89 45 d0 31 c0 48 8b 82 c0 06 00 00 48 8b 8a c8 06 00 00 <44> 8b 60 1c 48 89 4d a0 4c 89 e2 49 83 c4 20 48 83 fb 00 75 2a 49
> RSP: 0018:ffffa9c901203db0 EFLAGS: 00010246
> RAX: 0000000000000010 RBX: 0000000000000000 RCX: 0000000000000010
> RDX: ffff8ba1eb99c000 RSI: ffff8ba1e4ce8280 RDI: ffff8ba1e4ce8258
> RBP: ffffa9c901203e40 R08: ffffa9c901203dd8 R09: ffff8ba1ec443300
> R10: ffffa9c901203e50 R11: 0000000000000000 R12: ffff8ba1e4ce8280
> R13: ffffa9c901203ef0 R14: ffffa9c901203ef0 R15: ffff8ba1e4ce8258
> FS: 00007f6595460880(0000) GS:ffff8ba1ef880000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000000000002c CR3: 00000007d8d18003 CR4: 00000000003706e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ? __kmalloc_node+0x113/0x320
> ? kvmalloc_node+0x31/0x80
> seq_read+0x94/0x420
> vfs_read+0xa7/0x190
> ksys_read+0xa7/0xe0
> __x64_sys_read+0x1a/0x20
> do_syscall_64+0x37/0x80
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> In this situation, the bios_event_log pointer in the tpm_bios_log struct
> was not NULL but was equal to the ZERO_SIZE_PTR (0x10) value. This was
> due to the following kmemdup() in tpm_read_log_efi():
>
> int tpm_read_log_efi(struct tpm_chip *chip)
> {
> ...
> /* malloc EventLog space */
> log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
> if (!log->bios_event_log) {
> ret = -ENOMEM;
> goto out;
> }
> ...
> }
>
> When log_size is zero, due to an empty event log from firmware,
> ZERO_SIZE_PTR is returned from kmemdup(). Upon a read of the
> binary_bios_measurements file, the tpm2_bios_measurements_start()
> function does not perform a ZERO_OR_NULL_PTR() check on the
> bios_event_log pointer before dereferencing it.
>
> Rather than add a ZERO_OR_NULL_PTR() check in functions that make use of
> the bios_event_log pointer, simply avoid creating the
> binary_bios_measurements_file as is done in other event log retrieval
> backends.
>
> Explicitly ignore all of the events in the final event log when the main
> event log is empty. The list of events in the final event log cannot be
> accurately parsed without referring to the first event in the main event
> log (the event log header) so the final event log is useless in such a
> situation.
>
> Fixes: 58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table")
> Link: https://lore.kernel.org/linux-integrity/[email protected]/
> Reported-by: Kai-Heng Feng <[email protected]>
> Reported-by: Kenneth R. Crudup <[email protected]>
> Reported-by: Mimi Zohar <[email protected]>
> Cc: Thi?baud Weksteen <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Signed-off-by: Tyler Hicks <[email protected]>

While I was never able to get my firmware to provide me an empty event
log, I reproduced this issue locally and verified the fix by forcing the EFI
stub to create an empty event log like so:

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 7acbac16eae0..a9b9478cff14 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -85,7 +85,7 @@ void efi_retrieve_tpm2_eventlog(void)
/*
* We populate the EFI table even if the logs are empty.
*/
- if (!log_last_entry) {
+ if (true) {
log_size = 0;
} else {
last_entry_addr = (unsigned long) log_last_entry;

With that EFI stub change, I was able to verify that this crash happens
with or without commit 7f3d176f5f7e ("tpm: Require that all digests are
present in TCG_PCR_EVENT2 structures"). So, there's still the question
of why commit 7f3d176f5f7e uncovered this crash?

My theory, which I personally cannot prove since my Thinkpad's firmware
isn't acting the same as Kai's, Kenneth's, or Mimi's, is that some
EFI firmware is providing an empty main event log and a populated final
event log.

If this is true, prior to commit 7f3d176f5f7e, the EFI stub's
efi_retrieve_tpm2_eventlog() function would follow suite and provide the
same empty main event log and populated final event log to the kernel.

After commit 7f3d176f5f7e and when the main event log is empty, the
while loop below would start returning 0 because log_location is not a
valid event log header. It would either be zeroes or otherwise
junk/unexpected data (I'm not sure). The point is that final_events_size
is not incremented and the final event log is (rightfully, IMO) treated
as being empty after commit 7f3d176f5f7e.

void efi_retrieve_tpm2_eventlog(void)
{
...
/*
* Figure out whether any events have already been logged to the
* final events structure, and if so how much space they take up
*/
if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
if (final_events_table && final_events_table->nr_events) {
struct tcg_pcr_event2_head *header;
int offset;
void *data;
int event_size;
int i = final_events_table->nr_events;

data = (void *)final_events_table;
offset = sizeof(final_events_table->version) +
sizeof(final_events_table->nr_events);

while (i > 0) {
header = data + offset + final_events_size;
event_size = __calc_tpm2_event_size(header,
(void *)(long)log_location,
false);
final_events_size += event_size;
i--;
}
}
...
log_tbl->final_events_preboot_size = final_events_size;
...
}

Now we'll move to tpm_read_log_efi() to see why it matters whether or
not final_events_size is zero:

/* read binary bios log from EFI configuration table */
int tpm_read_log_efi(struct tpm_chip *chip)
{
...
efi_tpm_final_log_size -= log_tbl->final_events_preboot_size;

tmp = krealloc(log->bios_event_log,
log_size + efi_tpm_final_log_size,
GFP_KERNEL);
if (!tmp) {
kfree(log->bios_event_log);
ret = -ENOMEM;
goto out;
}

log->bios_event_log = tmp;

/*
* Copy any of the final events log that didn't also end up in the
* main log. Events can be logged in both if events are generated
* between GetEventLog() and ExitBootServices().
*/
memcpy((void *)log->bios_event_log + log_size,
final_tbl->events + log_tbl->final_events_preboot_size,
efi_tpm_final_log_size);
log->bios_event_log_end = log->bios_event_log +
log_size + efi_tpm_final_log_size;
...
}

There's nothing in tpm_read_log_efi(), prior to the patch I just
proposed, that prevents the final event log from being copied into an
otherwise empty main event log. In that situation, log->bios_event_log
is allocated (remember, it was not initially allocated because log_size
was 0) and the NULL pointer dereference crash upon reading the
binary_bios_measurements file would not happen.

The problem is that the resulting event log is not usable because
there's no event log header. __calc_tpm2_event_size() was too lax, prior
to commit 7f3d176f5f7e, to notice that the event log header was not
actually an event log header and, instead, was a regular event from the
final event log.

So, we need help from Kai, Kenneth, or Mimi to verify my assumption that
their firmware is providing an empty main event log and a populated
final event log. If that assumption is correct, then we could either:

A) Be alright with my proposed patch that bails out of
tpm_read_log_efi() when the main event log is empty. The crash would
be fixed and the binary_bios_measurements file simply would not
exist.

or

B) Write an additional, separate patch for the EFI stub that ignores the
final event log when the main event log is empty. Perhaps something
like this (untested):

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 7acbac16eae0..ad8f9ecf6dc2 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -127,7 +127,7 @@ void efi_retrieve_tpm2_eventlog(void)
* Figure out whether any events have already been logged to the
* final events structure, and if so how much space they take up
*/
- if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+ if (log_size && version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
if (final_events_table && final_events_table->nr_events) {
struct tcg_pcr_event2_head *header;

Tyler

2020-10-30 04:48:20

[permalink] [raw]

Subject: Re: [PATCH] tpm: efi: Don't create binary_bios_measurements file for an empty log

On Wed, Oct 28, 2020 at 10:41:02AM -0500, Tyler Hicks wrote:
> Mimic the pre-existing ACPI and Device Tree event log behavior by not
> creating the binary_bios_measurements file when the EFI TPM event log is
> empty.
>
> This fixes the following NULL pointer dereference that can occur when
> reading /sys/kernel/security/tpm0/binary_bios_measurements after the
> kernel received an empty event log from the firmware:
>
> BUG: kernel NULL pointer dereference, address: 000000000000002c
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 2 PID: 3932 Comm: fwupdtpmevlog Not tainted 5.9.0-00003-g629990edad62 #17
> Hardware name: LENOVO 20LCS03L00/20LCS03L00, BIOS N27ET38W (1.24 ) 11/28/2019
> RIP: 0010:tpm2_bios_measurements_start+0x3a/0x550
> Code: 54 53 48 83 ec 68 48 8b 57 70 48 8b 1e 65 48 8b 04 25 28 00 00 00 48 89 45 d0 31 c0 48 8b 82 c0 06 00 00 48 8b 8a c8 06 00 00 <44> 8b 60 1c 48 89 4d a0 4c 89 e2 49 83 c4 20 48 83 fb 00 75 2a 49
> RSP: 0018:ffffa9c901203db0 EFLAGS: 00010246
> RAX: 0000000000000010 RBX: 0000000000000000 RCX: 0000000000000010
> RDX: ffff8ba1eb99c000 RSI: ffff8ba1e4ce8280 RDI: ffff8ba1e4ce8258
> RBP: ffffa9c901203e40 R08: ffffa9c901203dd8 R09: ffff8ba1ec443300
> R10: ffffa9c901203e50 R11: 0000000000000000 R12: ffff8ba1e4ce8280
> R13: ffffa9c901203ef0 R14: ffffa9c901203ef0 R15: ffff8ba1e4ce8258
> FS: 00007f6595460880(0000) GS:ffff8ba1ef880000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000000000002c CR3: 00000007d8d18003 CR4: 00000000003706e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> ? __kmalloc_node+0x113/0x320
> ? kvmalloc_node+0x31/0x80
> seq_read+0x94/0x420
> vfs_read+0xa7/0x190
> ksys_read+0xa7/0xe0
> __x64_sys_read+0x1a/0x20
> do_syscall_64+0x37/0x80
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> In this situation, the bios_event_log pointer in the tpm_bios_log struct
> was not NULL but was equal to the ZERO_SIZE_PTR (0x10) value. This was
> due to the following kmemdup() in tpm_read_log_efi():
>
> int tpm_read_log_efi(struct tpm_chip *chip)
> {
> ...
> /* malloc EventLog space */
> log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
> if (!log->bios_event_log) {
> ret = -ENOMEM;
> goto out;
> }
> ...
> }
>
> When log_size is zero, due to an empty event log from firmware,
> ZERO_SIZE_PTR is returned from kmemdup(). Upon a read of the
> binary_bios_measurements file, the tpm2_bios_measurements_start()
> function does not perform a ZERO_OR_NULL_PTR() check on the
> bios_event_log pointer before dereferencing it.
>
> Rather than add a ZERO_OR_NULL_PTR() check in functions that make use of
> the bios_event_log pointer, simply avoid creating the
> binary_bios_measurements_file as is done in other event log retrieval
> backends.
>
> Explicitly ignore all of the events in the final event log when the main
> event log is empty. The list of events in the final event log cannot be
> accurately parsed without referring to the first event in the main event
> log (the event log header) so the final event log is useless in such a
> situation.
>
> Fixes: 58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table")
> Link: https://lore.kernel.org/linux-integrity/[email protected]/
> Reported-by: Kai-Heng Feng <[email protected]>
> Reported-by: Kenneth R. Crudup <[email protected]>
> Reported-by: Mimi Zohar <[email protected]>
> Cc: Thi?baud Weksteen <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Signed-off-by: Tyler Hicks <[email protected]>
> ---

Applied, thanks.

> drivers/char/tpm/eventlog/efi.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index 6bb023de17f1..35229e5143ca 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -41,6 +41,11 @@ int tpm_read_log_efi(struct tpm_chip *chip)
> log_size = log_tbl->size;
> memunmap(log_tbl);
>
> + if (!log_size) {
> + pr_warn("UEFI TPM log area empty\n");
> + return -EIO;
> + }
> +
> log_tbl = memremap(efi.tpm_log, sizeof(*log_tbl) + log_size,
> MEMREMAP_WB);
> if (!log_tbl) {
>
> base-commit: ed8780e3f2ecc82645342d070c6b4e530532e680
> --
> 2.25.1
>
>

/Jarkko

2020-10-30 07:02:44