Re: [RFC PATCH] tpm: Allow the TPM2 pcr_extend HMAC capability to be disabled on boot

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Thu Nov 7, 2024 at 5:14 AM EET, Mimi Zohar wrote:
> On Thu, 2024-11-07 at 03:55 +0200, Jarkko Sakkinen wrote:
> > On Thu Nov 7, 2024 at 3:07 AM EET, Mimi Zohar wrote:
> > > On Thu, 2024-11-07 at 02:03 +0200, Jarkko Sakkinen wrote:
> > > > On Thu Nov 7, 2024 at 1:52 AM EET, Mimi Zohar wrote:
> > > > > On Thu, 2024-11-07 at 01:22 +0200, Jarkko Sakkinen wrote:
> > > > > > On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> > > > > > > 
> > > > > > > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > > > > > > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > > > > > > 
> > > > > > > The random number generator reseed occurs in a kernel thread that fires
> > > > > > > about once a minute, so it doesn't show up in really any of the boot
> > > > > > > timings.  Plus even with sessions added, what there now isn't a
> > > > > > > significant overhead even to the running kernel given it's asynchronous
> > > > > > > and called infrequently.
> > > > > > 
> > > > > > Ah, right then we need the boot flag, and my earlier comments to the
> > > > > > parameter apply. I've never used IMA so I don't actually even know in
> > > > > > detail how it is using TPM.
> > > > > 
> > > > > Huh?  A simple explanation is that IMA-measurement maintains a measurement list,
> > > > > similar to the pre-boot event log.  Each IMA-measurement record extends the TPM
> > > > > PCR (default PCR 10).
> > > > > 
> > > > > Assuming IMA is enabled in the kernel, then just add "ima_policy=tcb" or
> > > > > "ima_policy=critical_data" on the boot command line.  To view the measurement
> > > > > records, cat <securityfs>/integrity/ima/ascii_runtime_measurements.  Normally
> > > > > the IMA policy specified on the boot command line is replaced with a finer
> > > > > grained custom policy.
> > > > 
> > > > I'll try to figure out how to test it regularly. And yeah we need the
> > > > flag obviously.
> > > > 
> > > > I have my (CI compatible) framework that I run regularly with upstream
> > > > that I've mentioned a few times earlier.
> > > > 
> > > > https://codeberg.org/jarkko/linux-tpmdd-test
> > > > 
> > > > How would I would make all files in /etc get to get the checksums, and
> > > > how can I generate legit and illegit change to some file in that tree?
> > > > 
> > > > No need to address how to implement that to my framework, I can figure
> > > > that out. I just would love throw something so that any performance
> > > > regressions will be catched right at the get go, i.e. before they
> > > > end up to the mainline.
> > > 
> > > Yes, I still need to look at it.  FYI, the IMA policy cannot be defined in terms
> > > of pathnames.  For testing, we've been loopback mounting a filesystem and
> > > defining policy rules based on the UUID of the filesystem.  If you're using
> > > SELinux, then rules can be defined in terms of SELinux labels. There are other
> > > methods of identifying files.  Ken's been working on new IMA documentation[1],
> > > which can be viewed here
> > > https://ima-doc.readthedocs.io/en/latest/ima-concepts.html .
> > > 
> > > Here are some examples as to how to locally verify the IMA measurement list and
> > > the boot aggregate.
> > > 
> > > 1. To locally verify the IMA measurement list matches TPM PCR-10, use evmctl
> > > (ima-evm-utils).  For example,
> > > 
> > > a. An IMA measurement list without integrity violations
> > > (/sys/kernel/security/ima/violations)
> > > 
> > > evmctl ima_measurement /sys/kernel/security/ima/binary_runtime_measurements
> > > 
> > > b. An IMA measurement list with integrity violations
> > > 
> > > evmctl ima_measurement --ignore-violations
> > > /sys/kernel/security/ima/binary_runtime_measurements
> > > 
> > > 2. To locally verify the 'boot_aggregate' record, the first record in the IMA
> > > measurement list, use "evmctl ima_boot_aggregate -v" and compare the resulting
> > > hash with the one in the boot_aggregate record.
> > 
> > Thanks! I write an issue based on this to my Codeberg repository, and
> > purge it once the time. I'll start by that and later on formalize
> > some commits or perhaps IMA specific buildroot config...
>
> Another important test would to be to make sure that IMA doesn't go into "TPM-
> bypass" mode, which happens when the TPM initialization is for some reason
> delayed.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/integrity/ima/ima_init.c#n124
> > As far as the patch goes, I thought that I refine the patch myself, and
> > save everyone's time and nervers from unnecessary reviews rounds. It
> > does not make any radical changes to the approach.
>
> Thanks
> > 
> > See https://lore.kernel.org/linux-integrity/20241107004708.108667-1-jarkko@kernel.org/
> > 
> > I cannot take reviewed/tested-by's from any of the authors but if you
> > can check that it works for you I can surely send it Linus without
> > further tags than three SOB's :-) That said happy to get at least
> > tested-by from someone.
>
> Our emails crossed.  I suggested removing the word "encrypted" throughout the
> patch, as pcr_extend isn't encrypted, just HMAC'ed.

Well me getting this wrong, i.e. not noticing that in that case SA_ENCRYPT
is not passed, is a sign that hmac is wrong choice... I.e. my mistake
proves an argument here ;-)


>
> I'll re-test first thing tomorrow morning. Does the module_param require a value
> or is specifying the name on the boot command line enough?

Module params is what you should use in LKM's unless you have special
reason to use __setup(). It's a kernel-wide global where as this flag
is TPM driver only.

You need to just namespace it with "tpm." when passing throug kernel
command-line i.e. "tpm.pcr_integrity=1" (or whatever, open for naming
suggestions).

BR, Jarkko