Message-ID: <6722bf6f-1a3f-ee9c-55e2-cf63c64266a9@gmx.de>
Date:   Wed, 5 May 2021 01:15:29 +0200
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Jarkko Sakkinen <jarkko@...nel.org>
Cc:     peterhuewe@....de, jgg@...pe.ca, stefanb@...ux.vnet.ibm.com,
        James.Bottomley@...senpartnership.com, keescook@...omium.org,
        jsnitsel@...hat.com, ml.linux@...oe.vision,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] tpm: Simplify locality handling

Hi,

On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> What the heck is "simplification" and what that has to do with fixing
> anything? I don't understand your terminology.


The intention for this patch is not to fix anything. Please read the cover
letter and the commit message.
This patch is about making the locality handling easier by not claiming/releasing
it multiple times over the driver life time, but claiming it once at driver
startup and only releasing it at driver shutdown.

Right now we have locality request/release combos in

- probe_itpm()
- tpm_tis_gen_interrupt()
- tpm_tis_core_init()
- tpm_chip_start()

and there is still one combo missing for

- tpm2_get_timeouts()

which is the reason why we get the "TPM returned invalid status" bug in case
of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, see
below).

And if we are going to enable interrupts, we have to introduce yet another combo,
for accessing the status register in the interrupt handler, since TPM 2.0
requires holding the locality for writing to the status register. That makes
6 different code places in which we take and release the locality.

With this patch applied we only take the locality at one place. Furthermore
with interrupts enabled we dont have to claim the locality for each handler
execution, saving us countless claim/release combinations at runtime.

Hence the term "simplification" which is perfectly justified IMO.

So again, this patch is "only" in preparation for the next patch when interrupts
are actually enabled and we would have to take the locality in the interrupt
handler without this patch.



> On Sat, May 01, 2021 at 03:57:25PM +0200, Lino Sanfilippo wrote:

>> WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
>> CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G        WC        5.11.0-rc2-LS-HOME+ #1
>> Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
>> Workqueue: events_unbound async_run_entry_fn
>> pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
>> pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> sp : ffffffc0127e38f0
>> x29: ffffffc0127e38f0 x28: ffffffc011238000
>> x27: 0000000000000016 x26: 000000000000017a
>> x25: 0000000000000014 x24: ffffff8047f60000
>> x23: 0000000000000000 x22: 0000000000000016
>> x21: ffffff8044e8a480 x20: 0000000000000000
>> x19: ffffffc011238000 x18: ffffffc011238948
>> x17: 0000000000000000 x16: 0000000000000000
>> x15: ffffffc01141be81 x14: ffffffffffffffff
>> x13: ffffffc01141be7e x12: ffffffffffffffff
>> x11: ffffff807fb797f0 x10: 00000000000019b0
>> x9 : ffffffc0127e38f0 x8 : 766e692064656e72
>> x7 : 0000000000000000 x6 : ffffffc011239000
>> x5 : ffffff807fb628b8 x4 : 0000000000000000
>> x3 : 0000000000000027 x2 : 0000000000000000
>> x1 : 6818b6f22fdef800 x0 : 0000000000000000
>> Call trace:
>> tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> tpm_tis_send_data+0x58/0x250 [tpm_tis_core]
>> tpm_tis_send_main+0x50/0x128 [tpm_tis_core]
>> tpm_tis_send+0x4c/0xe0 [tpm_tis_core]
>> tpm_transmit+0xd0/0x350 [tpm]
>> tpm_transmit_cmd+0x3c/0xc0 [tpm]
>> tpm2_get_tpm_pt+0x124/0x1e8 [tpm]
>> tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core]
>> tpm_tis_core_init+0x304/0x520 [tpm_tis_core]
>> tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi]
>> tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi]
>> tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi]
>> spi_probe+0x84/0xf0
>> really_probe+0x118/0x420
>> driver_probe_device+0x5c/0xc0
>> __driver_attach_async_helper+0x64/0x68
>> async_run_entry_fn+0x48/0x150
>> process_one_work+0x15c/0x4d0
>> worker_thread+0x50/0x490
>> kthread+0x118/0x150
>> ret_from_fork+0x10/0x1c
>>
>> The reason for this issue is that in case of TPM 2 function
>> tpm2_get_timeouts() which executes a TPM command is called without a
>> claimed locality. Since with this patch the locality is taken once at
>> driver startup and never released before shutdown the issue does not occur
>> any more.
>
> Please rather create fix that fixes the issue exactly in the code path.
> I don't want to worry what other things this might do "as a side-effect".

As explained above this patch is not meant to fix a bug in the first place
but rather fixes the bug above incidentally by the locality handling reimplementation.

> Also, lacks the explanation why this patch fixes the issue.
>

The explanation is there:

"The reason for this issue is that in case of TPM 2 function
 tpm2_get_timeouts() which executes a TPM command is called without a
 claimed locality. Since with this patch the locality is taken once at
 driver startup and never released before shutdown the issue does not occur
 any more."

I really dont know how to describe this more clear.


Lino

Powered by blists - more mailing lists