Ancestors

Toot

Written by Christian Heusel on 2024-10-21 at 13:12

@jarkko how likely is it that a change in the TPM subsystem leads to cracks in the headphones? 😆

I'm currrently struggling to belive the bisection results, even though they were double-checked 😄

=> More informations about this toot | More toots from gromit@chaos.social

Descendants

Written by Jarkko Sakkinen on 2024-10-21 at 16:07

@gromit i might actually might have hunch for this but have been unmotivated to move forward before these patches are merged. One thing at a time IMHO, especially with performance fixes.

NOTE: this is a hyphothesis.

tpm2_get_random() needs to be reconsidered with encryption as hwrng calls it often enough and there's now more overhead.

It pulls entropy in small chunks subtracting the length in the look like you would use read() syscall from user space. This is not right. This leads to small and variable size exchanges and yeah generally that equates to an inefficient implemenation with bad latency properties.

A better implementation would build on two concepts:

1. Have a pool "struct tpm2_random_pool" per caller with a fixed-size buffer in page granularity. E.g. hwrng end of TPM driver would have an instance. This would be exta argument for tpm2_get_random().

2. Have low and high watermarks (or thresholds) for triggering the pull from the chip so that it does not start to throttle.

Normally when the call is made entropy is served from the pool. When going below low water mark a thread (e.g. kthread, workqueue) is woken and less entropy is returned to the caller. Thread then fills the pool with fixes-size requests up until it goes above the high watermark.

I've posted my first patch set early on when the bug was recognized and also reporter confirmed that my fixes do in fact sort out the issue. There's been already some push to also sort some IMA issue but it is chaos if you start to include tons of random fixes to a single patch only because nobody pays any attention.

So I will work on that random pool patch after and also promise not to do anything before my first patch set has landed. With those ideas anyone who wants send me e.g. an RFC patch that I'm happy to give feedback.

=> More informations about this toot | More toots from jarkko@social.kernel.org

Written by Jarkko Sakkinen on 2024-10-21 at 16:08

@gromit Thanks for asking. I randomly noticed this with custom eBPF traces while debugging the boot-time issue. It is just a guess but this does add more and longer wait states here and there so theoretically could be related but obviously I cannot promise that.

=> More informations about this toot | More toots from jarkko@social.kernel.org

Written by Christian Heusel on 2024-10-21 at 16:15

@jarkko

tpm2_get_random() needs to be reconsidered with encryption as hwrng calls it often enough and there's now more overhead.

Yeah this would indeed make sense, as we landed on "tpm: add session encryption protection to tpm2_get_random()" as a bad commit ...

Is there already a report? If not I will send mine later 🤔

=> More informations about this toot | More toots from gromit@chaos.social