Re: [PATCH 00/13] x86: Trenchboot secure dynamic launch Linux kernel support
On Fri, Sep 25, 2020 at 05:32:50PM -0400, Daniel P. Smith wrote: > The work for this is split across different teams with different > resourcing levels resulting in one organization working Intel and > another working AMD. This then raised the concern over submitting a > single patch set developed by two groups pseudo-independently. In this > situation the result would be patches being submitted from one > organization that had no direct development or testing and therefore > could not sign off on a subset of the patches being submitted. Not sure if internal team structures qualify as a techical argument for upstream code. > > I'd be more motivated to review and test a full all encompassing x86 > > solution. It would increase the patch set size but would also give it > > a better test coverage, which I think would be a huge plus in such a > > complex patch set. > > We would not disagree with those sentiments but see the previous > response about the conflict that exists. At minimum, you have to make the case that the AMD support is easy to tackle in to the framework of things you have later on. /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/13] x86: Trenchboot secure dynamic launch Linux kernel support
On Thu, Sep 24, 2020 at 10:58:28AM -0400, Ross Philipson wrote: > The Trenchboot project focus on boot security has led to the enabling of > the Linux kernel to be directly invocable by the x86 Dynamic Launch > instruction(s) for establishing a Dynamic Root of Trust for Measurement > (DRTM). The dynamic launch will be initiated by a boot loader with What is "the dynamic launch"? > associated support added to it, for example the first targeted boot > loader will be GRUB2. An integral part of establishing the DRTM involves > measuring everything that is intended to be run (kernel image, initrd, > etc) and everything that will configure that kernel to run (command > line, boot params, etc) into specific PCRs, the DRTM PCRs (17-22), in > the TPM. Another key aspect is the dynamic launch is rooted in hardware, > that is to say the hardware (CPU) is what takes the first measurement > for the chain of integrity measurements. On Intel this is done using > the GETSEC instruction provided by Intel's TXT and the SKINIT > instruction provided by AMD's AMD-V. Information on these technologies > can be readily found online. This patchset introduces Intel TXT support. Why not both Intel and AMD? You should explain this in the cover letter. I'd be more motivated to review and test a full all encompassing x86 solution. It would increase the patch set size but would also give it a better test coverage, which I think would be a huge plus in such a complex patch set. > To enable the kernel to be launched by GETSEC, a stub must be built > into the setup section of the compressed kernel to handle the specific > state that the dynamic launch process leaves the BSP in. This is > analogous to the EFI stub that is found in the same area. Also this stub How is it analogous? > must measure everything that is going to be used as early as possible. > This stub code and subsequent code must also deal with the specific > state that the dynamic launch leaves the APs in. What is "the specific state"? > A quick note on terminology. The larger open source project itself is > called Trenchboot, which is hosted on Github (links below). The kernel > feature enabling the use of the x86 technology is referred to as "Secure > Launch" within the kernel code. As such the prefixes sl_/SL_ or > slaunch/SLAUNCH will be seen in the code. The stub code discussed above > is referred to as the SL stub. Is this only for Trenchboot? I'm a bit lost. What is it anyway? > The basic flow is: > > - Entry from the dynamic launch jumps to the SL stub > - SL stub fixes up the world on the BSP What is "SL"? > - For TXT, SL stub wakes the APs, fixes up their worlds > - For TXT, APs are left halted waiting for an NMI to wake them > - SL stub jumps to startup_32 > - SL main runs to measure configuration and module information into the >DRTM PCRs. It also locates the TPM event log. > - Kernel boot proceeds normally from this point. > - During early setup, slaunch_setup() runs to finish some validation >and setup tasks. What are "some" validation and setup tasks? > - The SMP bringup code is modified to wake the waiting APs. APs vector >to rmpiggy and start up normally from that point. > - Kernel boot finishes booting normally > - SL securityfs module is present to allow reading and writing of the >TPM event log. What is SL securityfs module? Why is it needed? We already have securityfs file for the event log. Why it needs to be writable? > - SEXIT support to leave SMX mode is present on the kexec path and >the various reboot paths (poweroff, reset, halt). What SEXIT do and why it is required on the kexec path? /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 05/13] x86: Add early TPM1.2/TPM2.0 interface support for Secure Launch
On Thu, Sep 24, 2020 at 10:58:33AM -0400, Ross Philipson wrote: > From: "Daniel P. Smith" > > This commit introduces an abstraction for TPM1.2 and TPM2.0 devices > above the TPM hardware interface. > > Signed-off-by: Daniel P. Smith > Signed-off-by: Ross Philipson This is way, way too PoC. I wonder why there is no RFC tag. Please also read section 2 of https://www.kernel.org/doc/html/v5.8/process/submitting-patches.html You should leverage existing TPM code in a way or another. Refine it so that it scales for your purpose and then compile it into your thing (just include the necesary C-files with relative paths). How it is now is never going to fly. /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 05/13] x86: Add early TPM1.2/TPM2.0 interface support for Secure Launch
On Tue, Sep 29, 2020 at 07:47:52PM -0400, Daniel P. Smith wrote: > TrenchBoot's AMD Secure Loader (LZ). The former is not well supported > and the latter will be getting maintenance under TB. While this is not > preferred, we had to weigh this versus trying to convince you and the > other TPM driver maintainers on a significant refactoring of the TPM > driver. It was elected for the reuse of a clean implementation that can > be replaced later if/when the TPM driver was refactored. When we > explained this during the RFC and it was not rejected, therefore we > carried it forward into this submission. What does it anyway mean when you say "RFC was not rejected"? I don't get the semantics of that sentence. It probably neither was ack'd, right? I do not really care what happened with the RFC. All I can say is that in the current state this totally PoC from top to bottom. > > How it is now is never going to fly. > > We would gladly work with you and the other TPM maintainers on a > refactoring of the TPM driver to separate core logic into standalone > files that both the driver and the compressed kernel can share. Yes, exactly. You have to refactor out the common parts. This is way too big patch to spend time on giving any more specific advice. Should be in way smaller chunks. For (almost) any possible, this is of unacceptable size. I think that it'd be best first to keep the common files in drivers/char/tpm and include them your code with relative paths in the Makefile. At least up until we have clear view what are the common parts. You might also want to refactor drivers/char/tpm/tpm.h and include/linux TPM headers to move more stuff into include/linux. If you are expecting a quick upstreaming process, please do not. This will take considerable amount of time to get right. /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 05/13] x86: Add early TPM1.2/TPM2.0 interface support for Secure Launch
On Wed, Sep 30, 2020 at 06:19:57AM +0300, Jarkko Sakkinen wrote: > On Tue, Sep 29, 2020 at 07:47:52PM -0400, Daniel P. Smith wrote: > > TrenchBoot's AMD Secure Loader (LZ). The former is not well supported > > and the latter will be getting maintenance under TB. While this is not > > preferred, we had to weigh this versus trying to convince you and the > > other TPM driver maintainers on a significant refactoring of the TPM > > driver. It was elected for the reuse of a clean implementation that can > > be replaced later if/when the TPM driver was refactored. When we > > explained this during the RFC and it was not rejected, therefore we > > carried it forward into this submission. > > What does it anyway mean when you say "RFC was not rejected"? I don't > get the semantics of that sentence. It probably neither was ack'd, > right? I do not really care what happened with the RFC. All I can say > is that in the current state this totally PoC from top to bottom. > > > > How it is now is never going to fly. > > > > We would gladly work with you and the other TPM maintainers on a > > refactoring of the TPM driver to separate core logic into standalone > > files that both the driver and the compressed kernel can share. > > Yes, exactly. You have to refactor out the common parts. This is way too > big patch to spend time on giving any more specific advice. Should be in > way smaller chunks. For (almost) any possible, this is of unacceptable ^ " patch" > size. > > I think that it'd be best first to keep the common files in > drivers/char/tpm and include them your code with relative paths in the > Makefile. At least up until we have clear view what are the common > parts. > > You might also want to refactor drivers/char/tpm/tpm.h and include/linux > TPM headers to move more stuff into include/linux. > > If you are expecting a quick upstreaming process, please do not. This > will take considerable amount of time to get right. /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: intel_iommu=on breaks resume from suspend on several Thinkpad models
On Sun, Sep 06, 2020 at 11:38:08PM -0400, Ronan Jouchet wrote: > Hi. This is a follow-up of [BUG] > https://bugzilla.kernel.org/show_bug.cgi?id=197029 , > where Jarkko Sakkinen asks in comment 31 to move discussion here. > > [1.] One line summary of the problem: > > intel_iommu=on breaks resume from suspend on several Thinkpad models > > [2.] Full description of the problem/report: > > With intel_iommu=on, on several Thinkpad models (my personal T560, and > the X1 Yoga / Yoga 460 of commenters over at [BUG]), suspend does work, > but pressing POWER / Enter / whatever key fails to resume from suspend. > > Instead, the machine doesn't do anything: system remains suspended, > the glowing LED keeps glowing, and the only option is to force a > hard shutdown with a long press on POWER, and start the system again. > > [3.] Keywords (i.e., modules, networking, kernel): > > suspend, resume, power management, laptop, lenovo, ibm, thinkpad, intel > > [4.] Kernel information > > [4.1.] Kernel version (from /proc/version): > > Linux version 5.8.7-arch1-1 (linux@archlinux) > (gcc (GCC) 10.2.0, GNU ld (GNU Binutils) 2.35) #1 SMP PREEMPT > Sat, 05Sep 2020 12:31:32 + > > This is the official `linux` package currently in Arch's `core` repo: > https://www.archlinux.org/packages/core/x86_64/linux/ > > [4.2.] Kernel .config file: > > https://github.com/archlinux/svntogit-packages/blob/packages/linux/trunk/config > > [5.] Most recent kernel version which did not have the bug: > > Undetermined. > > I witnessed the bug in Linux [ 4.13 , 5.8.7 ] but the bug predates 4.13. > I first noticed it in 4.13 because it's the first version where Arch > shipped a kernel enabling `intel_iommu=on` by default. > > Since then, following the Arch Linux sister bug report linked below at > [ARCH-BUG], Arch kernel packagers switched back to `intel_iommu=off`. > > [X. Other notes and bugzilla bug summary/chronology] > > X.1. This is a follow-up to these threads: > - [BUG] https://bugzilla.kernel.org/show_bug.cgi?id=197029 > - [ARCH-BBS] https://bbs.archlinux.org/viewtopic.php?pid=1737688 > - [ARCH-BUG] https://bugs.archlinux.org/task/55705 > > X.2. Over at [ARCH-BBS], someone suggested I try `intel_iommu=igfx_off` > rather than full `intel_iommu=off`. It's not enough; even with > `intel_iommu=igfx_off`, resume from suspend is broken. > > X.3. The same commenter over at [ARCH-BBS] suggests this bug might be > related to https://bugs.freedesktop.org/show_bug.cgi?id=89360 > > X.4. Problem was brought to the Linux IOMMU list: > > https://lists.linuxfoundation.org/pipermail/iommu/2017-September/024382.html > > X.5. Several Reddit commenters confirmed the problem: > > https://www.reddit.com/r/archlinux/comments/72z2rv/linux_41331_is_in_core/dnmjaeo/ > > X.6. On 2017-09-30, buzilla commenter Albert wrote at > https://bugzilla.kernel.org/show_bug.cgi?id=197029#c9 that: > > > I'm seeing this on my X1 Yoga (gen1) as well. > > > > When going to suspend (via systemctl suspend) with the default > > (intel_iommu=on), the power light starts fading/"breathing", > > but the audio mute LED stays on and the machine hangs. > > > > With intel_iommu=off, the power light breathes as well and the > > auto mute LED turns off correctly. I can then resume it normally > > (by pressing the Fn key). > > X.7. On 2017-10-16, Lu Baolu from Intel wrote at > https://bugzilla.kernel.org/show_bug.cgi?id=197029#c13 that: > > > This issue has been narrowed down to a hidden ME device which > > is not OS aware. The main symptom is below error log message > > and system fails to resume after being suspended. > > > > DMAR: DRHD: handling fault status reg 3 > > DMAR: [DMA Read] Request device [00:12.4] fault addr b7fff000 > > [fault reason 02] Present bit in context entry is clear > > > > A quick workaround is make PTP OS aware in BIOS configuration. > > It's likely at "PCH-FW Configuration"->"PTP aware OS". > > However, I couldn't find such an option in my T560's BIOS :-/ > > X.8. On 2017-11-13, Lu Baolu from Intel wrote at > https://bugzilla.kernel.org/show_bug.cgi?id=197029#c18 that: > > > This bug is still under investigation. We have narrowed it > > as a regression caused by a previous commit. > > The commit owner is now working on a fix. > > X.9. On 2020-02-03, to my followup requests, Lu Baolu wrote at > https://bugzilla.kernel.org/sho
Re: [PATCH v3 14/14] tpm: Allow locality 2 to be set when initializing the TPM for Secure Launch
On Mon, Aug 09, 2021 at 12:38:56PM -0400, Ross Philipson wrote:
> The Secure Launch MLE environment uses PCRs that are only accessible from
> the DRTM locality 2. By default the TPM drivers always initialize the
> locality to 0. When a Secure Launch is in progress, initialize the
> locality to 2.
>
> Signed-off-by: Ross Philipson
> ---
> drivers/char/tpm/tpm-chip.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ddaeceb..48b9351 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include "tpm.h"
>
> DEFINE_IDR(dev_nums_idr);
> @@ -34,12 +35,20 @@
>
> static int tpm_request_locality(struct tpm_chip *chip)
> {
> - int rc;
> + int rc, locality;
int locality;
int rc;
>
> if (!chip->ops->request_locality)
> return 0;
>
> - rc = chip->ops->request_locality(chip, 0);
> + if (slaunch_get_flags() & SL_FLAG_ACTIVE) {
> + dev_dbg(>dev, "setting TPM locality to 2 for MLE\n");
> + locality = 2;
> + } else {
> + dev_dbg(>dev, "setting TPM locality to 0\n");
> + locality = 0;
> + }
Please, remove dev_dbg()'s.
> +
> + rc = chip->ops->request_locality(chip, locality);
> if (rc < 0)
> return rc;
>
> --
> 1.8.3.1
/Jarkko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/14] x86/boot: Add missing handling of setup_indirect structures
On Mon, Aug 09, 2021 at 12:38:44PM -0400, Ross Philipson wrote:
> One of the two functions in ioremap.c that handles setup_data was
> missing the correct handling of setup_indirect structures.
What is "correct handling", and how was it broken?
What is 'setup_indirect'?
> Functionality missing from original commit:
Remove this sentence.
> commit b3c72fc9a78e (x86/boot: Introduce setup_indirect)
Should be.
Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
> Signed-off-by: Ross Philipson
> ---
> arch/x86/mm/ioremap.c | 21 +++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index ab74e4f..f2b34c5 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -669,17 +669,34 @@ static bool __init
> early_memremap_is_setup_data(resource_size_t phys_addr,
>
> paddr = boot_params.hdr.setup_data;
> while (paddr) {
> - unsigned int len;
> + unsigned int len, size;
>
> if (phys_addr == paddr)
> return true;
>
> data = early_memremap_decrypted(paddr, sizeof(*data));
> + size = sizeof(*data);
>
> paddr_next = data->next;
> len = data->len;
>
> - early_memunmap(data, sizeof(*data));
> + if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
> + early_memunmap(data, sizeof(*data));
> + return true;
> + }
> +
> + if (data->type == SETUP_INDIRECT) {
> + size += len;
> + early_memunmap(data, sizeof(*data));
> + data = early_memremap_decrypted(paddr, size);
> +
> + if (((struct setup_indirect *)data->data)->type !=
> SETUP_INDIRECT) {
> + paddr = ((struct setup_indirect
> *)data->data)->addr;
> + len = ((struct setup_indirect
> *)data->data)->len;
> + }
> + }
> +
> + early_memunmap(data, size);
>
> if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
> return true;
> --
> 1.8.3.1
>
>
/Jarkko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 00/14] x86: Trenchboot secure dynamic launch Linux kernel support
On Mon, Aug 09, 2021 at 12:38:42PM -0400, Ross Philipson wrote: > The focus of Trechboot project (https://github.com/TrenchBoot) is to > enhance the boot security and integrity. This requires the linux kernel ~ Linux How does it enhance it? The following sentence explains the requirements for the Linux kernel, i.e. it's a question without answer. And if there is no answer, there is no need to merge this. /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
