| Message ID | 20241102152226.2593598-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Alternative TPM patches for Trenchboot | expand |
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. So based on that usually a literal and unquestionable truth, when it comes to securing platforms, the next question is how to make a single atomic root of trust for Trenchboot. There is really only one answer I think of for this it to make slmodule part of the tpm_tis_core and also init order will be sorted out. I'll describe the steps forward. Step 1: declare and refactor that module into drivers/char/tpm/tpm_tis_slmodule.c and add this to the Makefile: ifdef CONFIG_SECURE_LAUNCH obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_slmodule.o endif Step 2: add 'int kernel_locality;' to struct tpm_tis_data. Step 3: implement tpm_tis_set_locality() internal function. Step 4: drop sysfs-patch completely (solution is not generic). BR, Jarkko
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. The RoT for DRTM is the CPU/microcode, and that is not split. I am going to assume that you are speaking about the delay between the time of collecting measurement to the time of storing measurement? As a refresher, an RTM trust chain is constructed using the transitive trust[1] process. As noted in the definition, the Linux kernel in this case is considered a group of functions that were all evaluated and considered functions to be equally part of the TCB. This means you are trusting actions at time interval M equally to an action taken at time interval N. If one attempts to construct an argument that claims this is invalid, that would mean all RTM trust chains constructed in this manner are invalidated, including SRTM aka SecureBoot. This means as long as the measurements are recorded before the TCB is extended again, then it does not matter if it is done at time M or time N. Bringing this back to SecureLaunch, there would only be an issue if slmodule could be built as an external loadable module, thus not being part of the "group of functions" measured and executed by the SINIT ACM. AFAICT, slmodule can only either be compiled in or out, not as a loadable module. If there is a path we missed that allows it to be built as a loadable module, then that needs correcting. Due to this comment, I went testing KCONFIG options and could not come up with a way for this to occur. I did see that we probably should change CONFIG_SECURE_LAUNCH dependency from TCG_TPM to TPM_TIS and TCG_CRB. Just to avoid an invalid configuration where the necessary interfaces were not present, leading to triggering a TXT reset of the platform. [1] Transitive trust (TCG D-RTM Architecture - Version 1.0.0) Also known as "inductive trust." In this process, the Root of Trust gives a trustworthy description of a second group of functions. Based on this description, an interested entity can determine the trust it is to place in this second group of functions. If the interested entity determines that the trust level of the second group of functions is acceptable, the trust boundary is extended from the Root of Trust to include the second group of functions. In this case, the process can be iterated. The second group of functions can give a trustworthy description of the third group of functions, etc. Transitive trust is used to provide a trustworthy description of platform characteristics. > So based on that usually a literal and unquestionable truth, when it > comes to securing platforms, the next question is how to make a single > atomic root of trust for Trenchboot. As mentioned above, there is no split currently. > There is really only one answer I think of for this it to make slmodule > part of the tpm_tis_core and also init order will be sorted out. Only if your assertion that it was split, which it is not. > I'll describe the steps forward. 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. v/r, dps
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. > v/r, > dps BR, Jarkko
On Mon Nov 4, 2024 at 1:18 PM EET, Jarkko Sakkinen 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. ... and ignoring fixes that took me almost one day to fully get together is neither. These address the awful commit messages, tpm_tis-only filtering and not allowing repetition in the calls. BR, Jarkko
On Mon, 4 Nov 2024 at 12:18, Jarkko Sakkinen <jarkko@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?
On Mon Nov 4, 2024 at 1:19 PM EET, Jarkko Sakkinen wrote: > > 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. > > ... and ignoring fixes that took me almost one day to fully get together > is neither. > > These address the awful commit messages, tpm_tis-only filtering and not > allowing repetition in the calls. Also considering early setup: it is not part of uapi. It can be reconsidered after the feature is landed as improvement (perhaps also easier to project then). I don't think TPM2_PolicyLocality potential conflict is important for kernel, and that is the only known race I know at this point. I don't really get the problem here. It's almost I like I should not have mentioned potential concurrency issue in order to not get slandered. BR, Jarkko
On Mon Nov 4, 2024 at 1:27 PM EET, Ard Biesheuvel wrote: > On Mon, 4 Nov 2024 at 12:18, Jarkko Sakkinen <jarkko@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? I provided patches with my sob's and spent time on making the delta absolute minimal to what exists already. If those are picked, I'm good. They are essentially drop-in replicas to the existing patches. BR, Jarkko
On 11/4/24 06:27, Ard Biesheuvel wrote: > On Mon, 4 Nov 2024 at 12:18, Jarkko Sakkinen <jarkko@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
On Mon, 4 Nov 2024 at 12:52, Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > On 11/4/24 06:27, Ard Biesheuvel wrote: > > On Mon, 4 Nov 2024 at 12:18, Jarkko Sakkinen <jarkko@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. > 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?
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
On 11/4/24 06:55, 'Ard Biesheuvel' via trenchboot-devel wrote: > On Mon, 4 Nov 2024 at 12:52, Daniel P. Smith > <dpsmith@apertussolutions.com> wrote: >> >> On 11/4/24 06:27, Ard Biesheuvel wrote: >>> On Mon, 4 Nov 2024 at 12:18, Jarkko Sakkinen <jarkko@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. >> > > But the inclusion of the secure launch module will force the TPM 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
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/685f3f00ddf88e961e2d861b7c783010774fe19d.camel@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
On Mon Nov 4, 2024 at 1:18 PM EET, Jarkko Sakkinen wrote: > 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. Still this sounds unrealistic given that this was tpm_tis only feature, and even that driver spans to total three different types of drivers: MMIO, SPI and I2C. It would be ridiculous amount of code pulled into early setup. If you still think that would make sense then you could migrate all the functionality under lib/ which would be called by both tpm_tis_core's drivers and Trenchboot. Anyway, if past me did that call, honestly, I do actually get it. It's not a counter-argument to a represented potential concurrency issue, which can cause issues with at least one TPM2 command, or like "it was caused by you because you thought it was a bad idea to accept tons of code to early setup" ;-) I can live with that concurrency issue as long as it is known decision not to support TPM2_PolicyLocality in the in-kernel use cases. Then my patches do address remaining issues and they can be picked given the sob's. If the concurrency issue is unacceptable, then I would merge slmodule to tpm_tis_core. It does solve the concurrency bug. I'm now done with this until new version or the series is applied with my patches. I think I've done enough effort to get this merged and not the contrary. BR, Jarkko
On 11/4/24 08:21, James Bottomley wrote: > 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/685f3f00ddf88e961e2d861b7c783010774fe19d.camel@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. 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
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
On 11/2/24 8:22 AM, Jarkko Sakkinen wrote: > This is my alternative patch set to the TPM patches included into > Trenchboot series v11. I don't mind to which tree these are > picked in the end. All the patches also have my sob's, so in that > sense things are also cleared up. > > At least slmodule needs to be patched in the series given that > tpm_chip_set_locality() returns zero on success. > > 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? > > Cc: Daniel P. Smith <dpsmith@apertussolutions.com> > Cc: Ross Philipson <ross.philipson@oracle.com> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > > Daniel P. Smith (2): > tpm, tpm_tis: Close all localities > tpm, tpm_tis: Address positive localities in > tpm_tis_request_locality() > > Ross Philipson (2): > tpm, tpm_tis: allow to set locality to a different value > tpm: sysfs: Show locality used by kernel > > drivers/char/tpm/tpm-chip.c | 33 ++++++++++++++++++++++++++++++++- > drivers/char/tpm/tpm-sysfs.c | 10 ++++++++++ > drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++++++++-- > include/linux/tpm.h | 10 ++++++++++ > 4 files changed, 68 insertions(+), 3 deletions(-) > Jarkko, Thank you for your efforts here - we appreciate it. I am testing your proposed patch set to see if it meets our needs and if we don't run into the earlier problem of leaving the TPM in a bad state when kexec'ing the next kernel. Ross
On 11/4/24 15:36, James Bottomley wrote: > 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? 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
On 11/2/24 8:22 AM, Jarkko Sakkinen wrote: > This is my alternative patch set to the TPM patches included into > Trenchboot series v11. I don't mind to which tree these are > picked in the end. All the patches also have my sob's, so in that > sense things are also cleared up. > > At least slmodule needs to be patched in the series given that > tpm_chip_set_locality() returns zero on success. > > 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? > > Cc: Daniel P. Smith <dpsmith@apertussolutions.com> > Cc: Ross Philipson <ross.philipson@oracle.com> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > > Daniel P. Smith (2): > tpm, tpm_tis: Close all localities > tpm, tpm_tis: Address positive localities in > tpm_tis_request_locality() > > Ross Philipson (2): > tpm, tpm_tis: allow to set locality to a different value > tpm: sysfs: Show locality used by kernel > > drivers/char/tpm/tpm-chip.c | 33 ++++++++++++++++++++++++++++++++- > drivers/char/tpm/tpm-sysfs.c | 10 ++++++++++ > drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++++++++-- > include/linux/tpm.h | 10 ++++++++++ > 4 files changed, 68 insertions(+), 3 deletions(-) > Jarkko, We have tested with this latest RFC patch set and it does what we need. Things also functioned correctly when we closed down the TXT DRTM and brought up a follow on kernel with kexec. So we are good with dropping our TPM patches and adopting these. The last question is do you want to take these in directly as a standalone patch set or do you want us to submit them with our next patch set (v12)? And for what it is worth if you want it: Tested-by: Ross Philipson <ross.philipson@oracle.com> Thanks again for your help. Ross
On Tue, 5 Nov 2024 at 01:52, <ross.philipson@oracle.com> wrote: > > On 11/2/24 8:22 AM, Jarkko Sakkinen wrote: > > This is my alternative patch set to the TPM patches included into > > Trenchboot series v11. I don't mind to which tree these are > > picked in the end. All the patches also have my sob's, so in that > > sense things are also cleared up. > > > > At least slmodule needs to be patched in the series given that > > tpm_chip_set_locality() returns zero on success. > > > > 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? > > > > Cc: Daniel P. Smith <dpsmith@apertussolutions.com> > > Cc: Ross Philipson <ross.philipson@oracle.com> > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > > Daniel P. Smith (2): > > tpm, tpm_tis: Close all localities > > tpm, tpm_tis: Address positive localities in > > tpm_tis_request_locality() > > > > Ross Philipson (2): > > tpm, tpm_tis: allow to set locality to a different value > > tpm: sysfs: Show locality used by kernel > > > > drivers/char/tpm/tpm-chip.c | 33 ++++++++++++++++++++++++++++++++- > > drivers/char/tpm/tpm-sysfs.c | 10 ++++++++++ > > drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++++++++-- > > include/linux/tpm.h | 10 ++++++++++ > > 4 files changed, 68 insertions(+), 3 deletions(-) > > > > Jarkko, > > We have tested with this latest RFC patch set and it does what we need. > Things also functioned correctly when we closed down the TXT DRTM and > brought up a follow on kernel with kexec. So we are good with dropping > our TPM patches and adopting these. The last question is do you want to > take these in directly as a standalone patch set or do you want us to > submit them with our next patch set (v12)? > > And for what it is worth if you want it: > > Tested-by: Ross Philipson <ross.philipson@oracle.com> > If the patches as proposed work for you, please incorporate them into your v12.
On 11/5/24 8:24 AM, Ard Biesheuvel wrote: > On Tue, 5 Nov 2024 at 01:52, <ross.philipson@oracle.com> wrote: >> >> On 11/2/24 8:22 AM, Jarkko Sakkinen wrote: >>> This is my alternative patch set to the TPM patches included into >>> Trenchboot series v11. I don't mind to which tree these are >>> picked in the end. All the patches also have my sob's, so in that >>> sense things are also cleared up. >>> >>> At least slmodule needs to be patched in the series given that >>> tpm_chip_set_locality() returns zero on success. >>> >>> 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? >>> >>> Cc: Daniel P. Smith <dpsmith@apertussolutions.com> >>> Cc: Ross Philipson <ross.philipson@oracle.com> >>> Cc: Ard Biesheuvel <ardb@kernel.org> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> >>> Daniel P. Smith (2): >>> tpm, tpm_tis: Close all localities >>> tpm, tpm_tis: Address positive localities in >>> tpm_tis_request_locality() >>> >>> Ross Philipson (2): >>> tpm, tpm_tis: allow to set locality to a different value >>> tpm: sysfs: Show locality used by kernel >>> >>> drivers/char/tpm/tpm-chip.c | 33 ++++++++++++++++++++++++++++++++- >>> drivers/char/tpm/tpm-sysfs.c | 10 ++++++++++ >>> drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++++++++-- >>> include/linux/tpm.h | 10 ++++++++++ >>> 4 files changed, 68 insertions(+), 3 deletions(-) >>> >> >> Jarkko, >> >> We have tested with this latest RFC patch set and it does what we need. >> Things also functioned correctly when we closed down the TXT DRTM and >> brought up a follow on kernel with kexec. So we are good with dropping >> our TPM patches and adopting these. The last question is do you want to >> take these in directly as a standalone patch set or do you want us to >> submit them with our next patch set (v12)? >> >> And for what it is worth if you want it: >> >> Tested-by: Ross Philipson <ross.philipson@oracle.com> >> > > If the patches as proposed work for you, please incorporate them into your v12. Ok will do, thanks. Ross
This is my alternative patch set to the TPM patches included into Trenchboot series v11. I don't mind to which tree these are picked in the end. All the patches also have my sob's, so in that sense things are also cleared up. At least slmodule needs to be patched in the series given that tpm_chip_set_locality() returns zero on success. 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? Cc: Daniel P. Smith <dpsmith@apertussolutions.com> Cc: Ross Philipson <ross.philipson@oracle.com> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Daniel P. Smith (2): tpm, tpm_tis: Close all localities tpm, tpm_tis: Address positive localities in tpm_tis_request_locality() Ross Philipson (2): tpm, tpm_tis: allow to set locality to a different value tpm: sysfs: Show locality used by kernel drivers/char/tpm/tpm-chip.c | 33 ++++++++++++++++++++++++++++++++- drivers/char/tpm/tpm-sysfs.c | 10 ++++++++++ drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++++++++-- include/linux/tpm.h | 10 ++++++++++ 4 files changed, 68 insertions(+), 3 deletions(-)