Re: [RFC PATCH 0/4] Alternative TPM patches for Trenchboot

[Daniel P. Smith]

On 11/4/24 06:27, Ard Biesheuvel wrote:
> On Mon, 4 Nov 2024 at 12:18, Jarkko Sakkinen <jar...@kernel.org> wrote:
>>
>> On Mon Nov 4, 2024 at 12:57 PM EET, Daniel P. Smith wrote:
>>> On 11/2/24 14:00, Jarkko Sakkinen wrote:
>>>> On Sat Nov 2, 2024 at 5:22 PM EET, Jarkko Sakkinen wrote:
>>>>> It is not really my problem but I'm also wondering how the
>>>>> initialization order is managed. What if e.g. IMA happens to
>>>>> initialize before slmodule?
>>>>
>>>> The first obvious observation from Trenchboot implementation is that it
>>>> is 9/10 times worst idea ever to have splitted root of trust. Here it
>>>> is realized by an LKM for slmodule.
>>>
>>> First, there is no conflict between IMA and slmodule. With your change
>>> to make locality switching a one shot, the only issue would be if IMA
>>> were to run first and issue a locality switch to Locality 0, thus
>>> blocking slmodule from switching to Locality 2. As for PCR usage, IMA
>>> uses the SRTM PCRs, which are completely accessible under Locality 2.
>>
>> Just pointing out a possible problem (e.g. with TPM2_PolicyLocality).
>>
>>> Honestly, a better path forward would be to revisit the issue that is
>>> driving most of that logic existing, which is the lack of a TPM
>>> interface code in the setup kernel. As a reminder, this issue is due to
>>> the TPM maintainers position that the only TPM code in the kernel can be
>>> the mainline driver. Which, unless something has changed, is impossible
>>> to compile into the setup kernel due to its use of mainline kernel
>>> constructs not present in the setup kernel.
>>
>> I don't categorically reject adding some code to early setup. We have
>> some shared code EFI stub but you have to explain your changes
>> proeprly. Getting rejection in some early version to some approach,
>> and being still pissed about that years forward is not really way
>> to go IMHO.
>>
>
> Daniel has been nothing but courteous and patient, and you've waited
> 11 revision to come up with some bikeshedding patches that don't
> materially improve anything.
>
> So commenting on Daniel's approach here is uncalled for.
>
> Can we please converge on this?
>
> Daniel - if no component can be built as a module, there should be no
> reason for the set_default_locality() hook to be exported to modules
> right? And do we even need a sysfs node to expose this information?

Hi Ard,

The only reason off the top of my head of why it was exported was to
support the fact that the tpm module itself could be built as a module,
not that we were looking for it to be done so.

As to sysfs, there is the TXT register content that we would like to
have exposed, and they should be readonly. For context to contrast with,
tboot user space utility txt-stat worked by trying to read the TXT
register address space via /dev/mem, think enough is said there. The
other purpose we used sysfs was management of the DRTM log. We used it
to provide a means to ensure the DRTM eventlog is extended when
measurements are sent to the DRTM PCRs and then to be able to retrieve
the final log.

v/r,
dps

[Ard Biesheuvel]

But the inclusion of the secure launch module will force the TPM
module to be builtin too, surely.

> As to sysfs, there is the TXT register content that we would like to
> have exposed, and they should be readonly. For context to contrast with,
> tboot user space utility txt-stat worked by trying to read the TXT
> register address space via /dev/mem, think enough is said there. The
> other purpose we used sysfs was management of the DRTM log. We used it
> to provide a means to ensure the DRTM eventlog is extended when
> measurements are sent to the DRTM PCRs and then to be able to retrieve
> the final log.
>

I was referring specifically to the read-write sysfs node that permits
user space to update the default TPM locality. Does it need to be
writable? And does it need to exist at all?

[Jarkko Sakkinen]

On Mon Nov 4, 2024 at 1:55 PM EET, Ard Biesheuvel wrote:
> But the inclusion of the secure launch module will force the TPM
> module to be builtin too, surely.

"config SECURE_LAUNCH" is bool => no export needed.

I have it in the patches too simply because missed it.

BR, Jarkko

[Daniel P. Smith]

Correct, I was meaning that TPM could be built as a module and since we
were extending its public interface, the thought is it would be proper
for us to make it exported. We have no requirement for it to be export
for our usage.

>> As to sysfs, there is the TXT register content that we would like to
>> have exposed, and they should be readonly. For context to contrast with,
>> tboot user space utility txt-stat worked by trying to read the TXT
>> register address space via /dev/mem, think enough is said there. The
>> other purpose we used sysfs was management of the DRTM log. We used it
>> to provide a means to ensure the DRTM eventlog is extended when
>> measurements are sent to the DRTM PCRs and then to be able to retrieve
>> the final log.
>>
>
> I was referring specifically to the read-write sysfs node that permits
> user space to update the default TPM locality. Does it need to be
> writable? And does it need to exist at all?

Right, sorry. As I recall, that was introduce due to the sequence of how
the TPM driver handled locality, moving back to Locality 0 after done
sending cmds. In the Oracle implementation, the initramfs takes
integrity measurements of the environment it is about to kexec into, eg.
target kernel, initramfs, file system, etc. Some of these measurements
should go into PCR 17 and PCR 18, which requires Locality 2 to be able
extend those PCRs. If the slmodule is able to set the locality for all
PCR extends coming from user space to be Locality 2, that removes the
current need for it.

v/r,
dps

[Daniel P. Smith]

Correct, I was meaning that TPM could be built as a module and since we
were extending its public interface, the thought is it would be proper
for us to make it exported. We have no requirement for it to be export
for our usage.

>> As to sysfs, there is the TXT register content that we would like to
>> have exposed, and they should be readonly. For context to contrast with,
>> tboot user space utility txt-stat worked by trying to read the TXT
>> register address space via /dev/mem, think enough is said there. The
>> other purpose we used sysfs was management of the DRTM log. We used it
>> to provide a means to ensure the DRTM eventlog is extended when
>> measurements are sent to the DRTM PCRs and then to be able to retrieve
>> the final log.
>>
>
> I was referring specifically to the read-write sysfs node that permits
> user space to update the default TPM locality. Does it need to be
> writable? And does it need to exist at all?

[James Bottomley]

On Mon, 2024-11-04 at 07:19 -0500, Daniel P. Smith wrote:
> On 11/4/24 06:55, 'Ard Biesheuvel' via trenchboot-devel wrote:

[...]

> > I was referring specifically to the read-write sysfs node that
> > permits user space to update the default TPM locality. Does it need
> > to be writable? And does it need to exist at all?

This was my question here, which never got answered as well:

https://lore.kernel.org/linux-integrity/685f3f00ddf88e961e2d861...@HansenPartnership.com/

> Right, sorry. As I recall, that was introduce due to the sequence of
> how the TPM driver handled locality, moving back to Locality 0 after
> done sending cmds. In the Oracle implementation, the initramfs takes
> integrity measurements of the environment it is about to kexec into,
> eg. target kernel, initramfs, file system, etc. Some of these
> measurements should go into PCR 17 and PCR 18, which requires
> Locality 2 to be able extend those PCRs. If the slmodule is able to
> set the locality for all PCR extends coming from user space to be
> Locality 2, that removes the current need for it.

Well, no, that's counter to the desire to have user space TPM commands
and kernel space TPM commands in different localities. I thought the
whole point of having locality restricted PCRs is so that only trusted
entities (i.e. those able to access the higher locality) could extend
into them. If you run every TPM command, regardless of source, in the
trusted locality, that makes the extends accessible to everyone and
thus destroys the trust boundary.

It also doesn't sound like the above that anything in user space
actually needs this facility. The measurements of kernel and initramfs
are already done by the boot stub (to PCR9, but that could be changed)
so we could do it all from the trusted entity.

Regards,

James

[Daniel P. Smith]

As to Locality switching:
The call sequence is,
tpm_pcr_extend -> tpm_find_get_ops -> tpm_try_get_ops ->
tpm_chip_start -> if (chip->locality == -1) tpm_request_locality
And when the extend completes:
out: tpm_put_ops -> tpm_chip_stop -> tpm_relinquish_locality ->
chip->locality = -1;

We made slmodule set the locality value used by request/relinquish back
to 0 when it was done with its initialization and then the sysfs nodes
to allow the runtime to request it when it needed to send measurements.
This is because we did not want to pin how it works to the one use case
currently focused on.

By definition I provided earlier, in our use case the initramfs is part
of the TCB as it is embedded into the kernel. As to the locality roles,
according to TPM Platform Profile:
- Locality 2: Dynamically Launched OS (Dynamic OS) “runtime” environment.
- Locality 1: An environment for use by the Dynamic OS.

> It also doesn't sound like the above that anything in user space
> actually needs this facility. The measurements of kernel and initramfs
> are already done by the boot stub (to PCR9, but that could be changed)
> so we could do it all from the trusted entity.

I apologies for not expressing this clearer, as that statement is
incorrect. The currently deployed use case works as follows:

[SRTM] --> [GRUB] -- (DLE, terminates SRTM chain) -->
[CPU] -- (starts DRTM chain) --> [SINIT ACM] -->
[SL kernel + initramfs] -- (load/measure/kexec) --> [target kernel]

As one can see, the SRTM is terminated and its components are not used
in the DRTM chain. This model reproduces the tboot model, with several
enhancements, including the ability for a single solution that supports
and works on Intel, AMD, and we are currently enabling Arm. It is not
the only model that can be used, which several were presented at 2020
Plumbers. A detailed version of a deployed implementation of the secure
upgrade use case was detailed in the 2021 FOSSDEM presentation. Where
the LCP policy is used to tell the ACM what [SL kernel + initramfs] are
allowed to be started by TXT. This allows the ability to launch into an
upgrade state without having to reboot.

In case the question comes up from those not familiar, the kexec does an
GETSEC[SEXIT] which closes off access to Localities 1 and 2, thus
locking the DRTM PCR values. It brings the CPUs out of SMX mode so the
target kernel does not require to have any knowledge about running in
that mode.

v/r,
dps

[James Bottomley]

On Mon, 2024-11-04 at 11:34 -0500, Daniel P. Smith wrote:
[...]

> In case the question comes up from those not familiar, the kexec does
> an GETSEC[SEXIT] which closes off access to Localities 1 and 2, thus
> locking the DRTM PCR values. It brings the CPUs out of SMX mode so
> the target kernel does not require to have any knowledge about
> running in that mode.

So, to repeat the question: why a sysfs interface for setting the
default locality? If I understand correctly from what you say above,
it can't be used in any kernel except the SL one, and that one could
run permanently in it, so there's no requirement at all for user space
to be able to change this, is there?

Regards,

James

[Daniel P. Smith]

I responded to Ard this morning that, "If the slmodule is able to set

the locality for all PCR extends coming from user space to be Locality

2, that removes the current need for it." Where "it" is the sysfs node
for default locality. This series does just that, so in a more direct
response, no, a writable sysfs node is no longer needed with this series.

v/r
dps