Re: [PATCH v9 04/19] x86: Secure Launch Resource Table header file
On Thu Jun 6, 2024 at 7:49 PM EEST, wrote: > > For any architectures dig a similar fact: > > > > 1. Is not dead. > > 2. Will be there also in future. > > > > Make any architecture existentially relevant for and not too much > > coloring in the text that is easy to check. > > > > It is nearing 5k lines so you should be really good with measured > > facts too (not just launch) :-) > > ... but overall I get your meaning. We will spend time on this sort of > documentation for the v10 release. Yeah, I mean we live in the universe of 3 letter acronyms so it is better to summarize the existential part, especially in a ~5 KSLOC patch set ;-) BR, Jarkko
Re: [PATCH v9 04/19] x86: Secure Launch Resource Table header file
On Wed Jun 5, 2024 at 10:03 PM EEST, wrote: > So I did not mean to imply that DRTM support on various > platforms/architectures has a short expiration date. In fact we are > actively working on DRTM support through the TrenchBoot project on > several platforms/architectures. Just a quick rundown here: > > Intel: Plenty of Intel platforms are vPro with TXT. It is really just > the lower end systems that don't have it available (like Core i3). And > my guess was wrong about x86s. You can find the spec on the page in the > following link. There is an entire subsection on SMX support on x86s and > the changes to the various GETSEC instruction leaves that were made to > make it work there (see 3.15). > > https://www.intel.com/content/www/us/en/developer/articles/technical/envisioning-future-simplified-architecture.html Happend to bump into same PDF specification and exactly the seeked information is "3.15 SMX Changes". So just write this down to some patch that starts adding SMX things. Link: https://cdrdv2.intel.com/v1/dl/getContent/776648 So link and document, and other stuff above is not relevant from upstream context, only potential maintenance burden :-) For any architectures dig a similar fact: 1. Is not dead. 2. Will be there also in future. Make any architecture existentially relevant for and not too much coloring in the text that is easy to check. It is nearing 5k lines so you should be really good with measured facts too (not just launch) :-) BR, Jarkko
Re: [PATCH v9 04/19] x86: Secure Launch Resource Table header file
On Wed Jun 5, 2024 at 5:33 AM EEST, wrote: > On 6/4/24 5:22 PM, Jarkko Sakkinen wrote: > > On Wed Jun 5, 2024 at 2:00 AM EEST, wrote: > >> On 6/4/24 3:36 PM, Jarkko Sakkinen wrote: > >>> On Tue Jun 4, 2024 at 11:31 PM EEST, wrote: > >>>> On 6/4/24 11:21 AM, Jarkko Sakkinen wrote: > >>>>> On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > >>>>>> Introduce the Secure Launch Resource Table which forms the formal > >>>>>> interface between the pre and post launch code. > >>>>>> > >>>>>> Signed-off-by: Ross Philipson > >>>>> > >>>>> If a uarch specific, I'd appreciate Intel SDM reference here so that I > >>>>> can look it up and compare. Like in section granularity. > >>>> > >>>> This table is meant to not be architecture specific though it can > >>>> contain architecture specific sub-entities. E.g. there is a TXT specific > >>>> table and in the future there will be an AMD and ARM one (and hopefully > >>>> some others). I hope that addresses what you are pointing out or maybe I > >>>> don't fully understand what you mean here... > >>> > >>> At least Intel SDM has a definition of any possible architecture > >>> specific data structure. It is handy to also have this available > >>> in inline comment for any possible such structure pointing out the > >>> section where it is defined. > >> > >> The TXT specific structure is not defined in the SDM or the TXT dev > >> guide. Part of it is driven by requirements in the TXT dev guide but > >> that guide does not contain implementation details. > >> > >> That said, if you would like links to relevant documents in the comments > >> before arch specific structures, I can add them. > > > > Vol. 2D 7-40, in the description of GETSEC[WAKEUP] there is in fact a > > description of MLE JOINT structure at least: > > > > 1. GDT limit (offset 0) > > 2. GDT base (offset 4) > > 3. Segment selector initializer (offset 8) > > 4. EIP (offset 12) > > > > So is this only exercised in protect mode, and not in long mode? Just > > wondering whether I should make a bug report on this for SDM or not. > > I believe you can issue the SENTER instruction in long mode, compat mode > or protected mode. On the other side thought, you will pop out of the > TXT initialization in protected mode. The SDM outlines what registers > will hold what values and what is valid and not valid. The APs will also > vector through the join structure mentioned above to the location > specified in protected mode using the GDT information you provide. > > > > > Especially this puzzles me, given that x86s won't have protected > > mode in the first place... > > My guess is the simplified x86 architecture will not support TXT. It is > not supported on a number of CPUs/chipsets as it stands today. Just a > guess but we know only vPro systems support TXT today. I'm wondering could this bootstrap itself inside TDX or SNP, and that way provide path forward? AFAIK, TDX can be nested straight of the bat and SNP from 2nd generation EPYC's, which contain the feature. I do buy the idea of attesting the host, not just the guests, even in the "confidential world". That said, I'm not sure does it make sense to add all this infrastructure for a technology with such a short expiration date? I would not want to say this at v9, and it is not really your fault either, but for me this would make a lot more sense if the core of Trenchboot was redesigned around these newer technologies with a long-term future. The idea itself is great! BR, Jarkko
Re: [PATCH v9 04/19] x86: Secure Launch Resource Table header file
On Wed Jun 5, 2024 at 3:22 AM EEST, Jarkko Sakkinen wrote: > On Wed Jun 5, 2024 at 2:00 AM EEST, wrote: > > On 6/4/24 3:36 PM, Jarkko Sakkinen wrote: > > > On Tue Jun 4, 2024 at 11:31 PM EEST, wrote: > > >> On 6/4/24 11:21 AM, Jarkko Sakkinen wrote: > > >>> On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > > >>>> Introduce the Secure Launch Resource Table which forms the formal > > >>>> interface between the pre and post launch code. > > >>>> > > >>>> Signed-off-by: Ross Philipson > > >>> > > >>> If a uarch specific, I'd appreciate Intel SDM reference here so that I > > >>> can look it up and compare. Like in section granularity. > > >> > > >> This table is meant to not be architecture specific though it can > > >> contain architecture specific sub-entities. E.g. there is a TXT specific > > >> table and in the future there will be an AMD and ARM one (and hopefully > > >> some others). I hope that addresses what you are pointing out or maybe I > > >> don't fully understand what you mean here... > > > > > > At least Intel SDM has a definition of any possible architecture > > > specific data structure. It is handy to also have this available > > > in inline comment for any possible such structure pointing out the > > > section where it is defined. > > > > The TXT specific structure is not defined in the SDM or the TXT dev > > guide. Part of it is driven by requirements in the TXT dev guide but > > that guide does not contain implementation details. > > > > That said, if you would like links to relevant documents in the comments > > before arch specific structures, I can add them. > > Vol. 2D 7-40, in the description of GETSEC[WAKEUP] there is in fact a > description of MLE JOINT structure at least: > > 1. GDT limit (offset 0) > 2. GDT base (offset 4) > 3. Segment selector initializer (offset 8) > 4. EIP (offset 12) > > So is this only exercised in protect mode, and not in long mode? Just > wondering whether I should make a bug report on this for SDM or not. > > Especially this puzzles me, given that x86s won't have protected > mode in the first place... That raises a relevant question: will this ever work in x86s? SDM does not really support that it would but it could be also just outdated information. I'm neither sure how or will AMD align with x86s. Just point out a glitch... BR, Jarkko
Re: [PATCH v9 04/19] x86: Secure Launch Resource Table header file
On Wed Jun 5, 2024 at 2:00 AM EEST, wrote: > On 6/4/24 3:36 PM, Jarkko Sakkinen wrote: > > On Tue Jun 4, 2024 at 11:31 PM EEST, wrote: > >> On 6/4/24 11:21 AM, Jarkko Sakkinen wrote: > >>> On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > >>>> Introduce the Secure Launch Resource Table which forms the formal > >>>> interface between the pre and post launch code. > >>>> > >>>> Signed-off-by: Ross Philipson > >>> > >>> If a uarch specific, I'd appreciate Intel SDM reference here so that I > >>> can look it up and compare. Like in section granularity. > >> > >> This table is meant to not be architecture specific though it can > >> contain architecture specific sub-entities. E.g. there is a TXT specific > >> table and in the future there will be an AMD and ARM one (and hopefully > >> some others). I hope that addresses what you are pointing out or maybe I > >> don't fully understand what you mean here... > > > > At least Intel SDM has a definition of any possible architecture > > specific data structure. It is handy to also have this available > > in inline comment for any possible such structure pointing out the > > section where it is defined. > > The TXT specific structure is not defined in the SDM or the TXT dev > guide. Part of it is driven by requirements in the TXT dev guide but > that guide does not contain implementation details. > > That said, if you would like links to relevant documents in the comments > before arch specific structures, I can add them. Vol. 2D 7-40, in the description of GETSEC[WAKEUP] there is in fact a description of MLE JOINT structure at least: 1. GDT limit (offset 0) 2. GDT base (offset 4) 3. Segment selector initializer (offset 8) 4. EIP (offset 12) So is this only exercised in protect mode, and not in long mode? Just wondering whether I should make a bug report on this for SDM or not. Especially this puzzles me, given that x86s won't have protected mode in the first place... BR, Jarkko
Re: [PATCH v9 16/19] tpm: Add ability to set the preferred locality the TPM chip uses
On Wed Jun 5, 2024 at 1:14 AM EEST, wrote:
> On 6/4/24 1:27 PM, Jarkko Sakkinen wrote:
> > On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> >> Curently the locality is hard coded to 0 but for DRTM support, access
> >> is needed to localities 1 through 4.
> >>
> >> Signed-off-by: Ross Philipson
> >> ---
> >> drivers/char/tpm/tpm-chip.c | 24 +++-
> >> drivers/char/tpm/tpm-interface.c | 15 +++
> >> drivers/char/tpm/tpm.h | 1 +
> >> include/linux/tpm.h | 4
> >> 4 files changed, 43 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> >> index 854546000c92..73eac54d61fb 100644
> >> --- a/drivers/char/tpm/tpm-chip.c
> >> +++ b/drivers/char/tpm/tpm-chip.c
> >> @@ -44,7 +44,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
> >>if (!chip->ops->request_locality)
> >>return 0;
> >>
> >> - rc = chip->ops->request_locality(chip, 0);
> >> + rc = chip->ops->request_locality(chip, chip->pref_locality);
> >>if (rc < 0)
> >>return rc;
> >>
> >> @@ -143,6 +143,27 @@ void tpm_chip_stop(struct tpm_chip *chip)
> >> }
> >> EXPORT_SYMBOL_GPL(tpm_chip_stop);
> >>
> >> +/**
> >> + * tpm_chip_preferred_locality() - set the TPM chip preferred locality to
> >> open
> >> + * @chip: a TPM chip to use
> >> + * @locality: the preferred locality
> >> + *
> >> + * Return:
> >> + * * true - Preferred locality set
> >> + * * false - Invalid locality specified
> >> + */
> >> +bool tpm_chip_preferred_locality(struct tpm_chip *chip, int locality)
> >> +{
> >> + if (locality < 0 || locality >=TPM_MAX_LOCALITY)
> >> + return false;
> >> +
> >> + mutex_lock(>tpm_mutex);
> >> + chip->pref_locality = locality;
> >> + mutex_unlock(>tpm_mutex);
> >> + return true;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tpm_chip_preferred_locality);
> >> +
> >> /**
> >>* tpm_try_get_ops() - Get a ref to the tpm_chip
> >>* @chip: Chip to ref
> >> @@ -374,6 +395,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
> >>}
> >>
> >>chip->locality = -1;
> >> + chip->pref_locality = 0;
> >>return chip;
> >>
> >> out:
> >> diff --git a/drivers/char/tpm/tpm-interface.c
> >> b/drivers/char/tpm/tpm-interface.c
> >> index 5da134f12c9a..35f14ccecf0e 100644
> >> --- a/drivers/char/tpm/tpm-interface.c
> >> +++ b/drivers/char/tpm/tpm-interface.c
> >> @@ -274,6 +274,21 @@ int tpm_is_tpm2(struct tpm_chip *chip)
> >> }
> >> EXPORT_SYMBOL_GPL(tpm_is_tpm2);
> >>
> >> +/**
> >> + * tpm_preferred_locality() - set the TPM chip preferred locality to open
> >> + * @chip: a TPM chip to use
> >> + * @locality: the preferred locality
> >> + *
> >> + * Return:
> >> + * * true - Preferred locality set
> >> + * * false - Invalid locality specified
> >> + */
> >> +bool tpm_preferred_locality(struct tpm_chip *chip, int locality)
> >> +{
> >> + return tpm_chip_preferred_locality(chip, locality);
> >> +}
> >> +EXPORT_SYMBOL_GPL(tpm_preferred_locality);
> >
> > What good does this extra wrapping do?
> >
> > tpm_set_default_locality() and default_locality would make so much more
> > sense in any case.
>
> Are you mainly just talking about my naming choices here and in the
> follow-on response? Can you clarify what you are requesting?
I'd prefer:
1. Name the variable as default_locality.
2. Only create a single expored to function to tpm-chip.c:
tpm_chip_set_default_locality().
3. Call this function in all call sites.
"tpm_preferred_locality" should be just removed, as tpm_chip_*
is exported anyway.
BR, Jarkko
Re: [PATCH v9 10/19] x86: Secure Launch SMP bringup support
On Wed Jun 5, 2024 at 12:47 AM EEST, wrote:
> > static inline bool slaunch_is_txt_launch(void)
> > {
> > u32 mask = SL_FLAG_ACTIVE | SL_FLAG_ARCH_TXT;
> >
> > return slaunch_get_flags() & mask == mask;
> > }
>
> Actually I think I can take your suggested change and move this function
> to the main header files since this check is done elsewhere. And later I
> can make others like slaunch_is_skinit_launch(). Thanks.
Yep, makes sense to me.
BR, Jarkko
Re: [PATCH v9 09/19] x86: Secure Launch kernel late boot stub
On Wed Jun 5, 2024 at 12:16 AM EEST, wrote: > On 6/4/24 12:58 PM, Jarkko Sakkinen wrote: > > On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > >> The routine slaunch_setup is called out of the x86 specific setup_arch() > >> routine during early kernel boot. After determining what platform is > >> present, various operations specific to that platform occur. This > >> includes finalizing setting for the platform late launch and verifying > >> that memory protections are in place. > > > > "memory protections" is not too helpful tbh. > > > > Better to describe very briefly the VT-d usage. > > We can enhance the commit message and talk about VT-d usage and what > PMRs are and do. Yep, pointing out exact things that you're dealing with is even more useful once the feature has landed. After some months one tends to start forgetting things, so it is good to use commit messages as clues and reminders of essential concepts. BR, Jarkko
Re: [PATCH v9 08/19] x86: Secure Launch kernel early boot stub
> > s/tpm20/tpm2/ > > Reasonable. We can change it. For the sake of consistency. Anywhere else where we have code using TPM, either "tpm_" or "tpm2_" is used. BR, Jarkko
Re: [PATCH v9 06/19] x86: Add early SHA-1 support for Secure Launch early measurements
On Wed Jun 5, 2024 at 12:02 AM EEST, wrote:
> On 6/4/24 11:52 AM, Jarkko Sakkinen wrote:
> > On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> >> From: "Daniel P. Smith"
> >>
> >> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
> >> choice of hashes used lie with the platform firmware, not with
> >> software, and is often outside of the users control.
> >>
> >> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
> >> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
> >> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
> >> to safely use SHA-256 for everything else.
> >>
> >> The SHA-1 code here has its origins in the code from the main kernel:
> >>
> >> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
> >>
> >> A modified version of this code was introduced to the lib/crypto/sha1.c
> >> to bring it in line with the SHA-256 code and allow it to be pulled into
> >> the
> >> setup kernel in the same manner as SHA-256 is.
> >>
> >> Signed-off-by: Daniel P. Smith
> >> Signed-off-by: Ross Philipson
> >> ---
> >> arch/x86/boot/compressed/Makefile | 2 +
> >> arch/x86/boot/compressed/early_sha1.c | 12
> >> include/crypto/sha1.h | 1 +
> >> lib/crypto/sha1.c | 81 +++
> >> 4 files changed, 96 insertions(+)
> >> create mode 100644 arch/x86/boot/compressed/early_sha1.c
> >>
> >> diff --git a/arch/x86/boot/compressed/Makefile
> >> b/arch/x86/boot/compressed/Makefile
> >> index e9522c6893be..3307ebef4e1b 100644
> >> --- a/arch/x86/boot/compressed/Makefile
> >> +++ b/arch/x86/boot/compressed/Makefile
> >> @@ -118,6 +118,8 @@ vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
> >> vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
> >> vmlinux-objs-$(CONFIG_EFI_STUB) +=
> >> $(objtree)/drivers/firmware/efi/libstub/lib.a
> >>
> >> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o
> >> +
> >> $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
> >>$(call if_changed,ld)
> >>
> >> diff --git a/arch/x86/boot/compressed/early_sha1.c
> >> b/arch/x86/boot/compressed/early_sha1.c
> >> new file mode 100644
> >> index ..8a9b904a73ab
> >> --- /dev/null
> >> +++ b/arch/x86/boot/compressed/early_sha1.c
> >> @@ -0,0 +1,12 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2024 Apertus Solutions, LLC.
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include "../../../../lib/crypto/sha1.c"
> > }
> >
> > Yep, make sense. Thinking only that should this be just sha1.c.
> >
> > Comparing this to mainly drivers/firmware/efi/tpm.c, which is not
> > early_tpm.c where the early actually probably would make more sense
> > than here. Here sha1 primitive is just needed.
> >
> > This is definitely a nitpick but why carry a prefix that is not
> > that useful, right?
>
> I am not 100% sure what you mean here, sorry. Could you clarify about
> the prefix? Do you mean why did we choose early_*? There was precedent
> for doing that like early_serial_console.c.
Yep, that exactly. I'd just name as sha1.c.
>
> >
> >> diff --git a/include/crypto/sha1.h b/include/crypto/sha1.h
> >> index 044ecea60ac8..d715dd5332e1 100644
> >> --- a/include/crypto/sha1.h
> >> +++ b/include/crypto/sha1.h
> >> @@ -42,5 +42,6 @@ extern int crypto_sha1_finup(struct shash_desc *desc,
> >> const u8 *data,
> >> #define SHA1_WORKSPACE_WORDS 16
> >> void sha1_init(__u32 *buf);
> >> void sha1_transform(__u32 *digest, const char *data, __u32 *W);
> >> +void sha1(const u8 *data, unsigned int len, u8 *out);
> >>
> >> #endif /* _CRYPTO_SHA1_H */
> >> diff --git a/lib/crypto/sha1.c b/lib/crypto/sha1.c
> >> index 1aebe7be9401..10152125b338 100644
> >> --- a/lib/crypto/sha1.c
> >> +++ b/lib/crypto/sha1.c
> >> @@ -137,4 +137,85 @@ void sha1_init(__u32 *buf)
> >> }
> >> EXPORT_SYMBOL(sha1_init);
> >>
> >> +static void __sha1_transform(u32 *digest, const char *data)
> >> +{
&g
Re: [PATCH v9 04/19] x86: Secure Launch Resource Table header file
On Tue Jun 4, 2024 at 11:31 PM EEST, wrote: > On 6/4/24 11:21 AM, Jarkko Sakkinen wrote: > > On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > >> Introduce the Secure Launch Resource Table which forms the formal > >> interface between the pre and post launch code. > >> > >> Signed-off-by: Ross Philipson > > > > If a uarch specific, I'd appreciate Intel SDM reference here so that I > > can look it up and compare. Like in section granularity. > > This table is meant to not be architecture specific though it can > contain architecture specific sub-entities. E.g. there is a TXT specific > table and in the future there will be an AMD and ARM one (and hopefully > some others). I hope that addresses what you are pointing out or maybe I > don't fully understand what you mean here... At least Intel SDM has a definition of any possible architecture specific data structure. It is handy to also have this available in inline comment for any possible such structure pointing out the section where it is defined. BR, Jarkko
Re: [PATCH v9 17/19] tpm: Add sysfs interface to allow setting and querying the preferred locality
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> Expose a sysfs interface to allow user mode to set and query the preferred
> locality for the TPM chip.
>
> Signed-off-by: Ross Philipson
> ---
> drivers/char/tpm/tpm-sysfs.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
> index 94231f052ea7..5f4a966a4599 100644
> --- a/drivers/char/tpm/tpm-sysfs.c
> +++ b/drivers/char/tpm/tpm-sysfs.c
> @@ -324,6 +324,34 @@ static ssize_t null_name_show(struct device *dev, struct
> device_attribute *attr,
> static DEVICE_ATTR_RO(null_name);
> #endif
>
> +static ssize_t preferred_locality_show(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + struct tpm_chip *chip = to_tpm_chip(dev);
> +
> + return sprintf(buf, "%d\n", chip->pref_locality);
> +}
Disagree with the naming.
BR, Jarkko
Re: [PATCH v9 16/19] tpm: Add ability to set the preferred locality the TPM chip uses
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> Curently the locality is hard coded to 0 but for DRTM support, access
> is needed to localities 1 through 4.
>
> Signed-off-by: Ross Philipson
> ---
> drivers/char/tpm/tpm-chip.c | 24 +++-
> drivers/char/tpm/tpm-interface.c | 15 +++
> drivers/char/tpm/tpm.h | 1 +
> include/linux/tpm.h | 4
> 4 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 854546000c92..73eac54d61fb 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -44,7 +44,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
> if (!chip->ops->request_locality)
> return 0;
>
> - rc = chip->ops->request_locality(chip, 0);
> + rc = chip->ops->request_locality(chip, chip->pref_locality);
> if (rc < 0)
> return rc;
>
> @@ -143,6 +143,27 @@ void tpm_chip_stop(struct tpm_chip *chip)
> }
> EXPORT_SYMBOL_GPL(tpm_chip_stop);
>
> +/**
> + * tpm_chip_preferred_locality() - set the TPM chip preferred locality to
> open
> + * @chip:a TPM chip to use
> + * @locality: the preferred locality
> + *
> + * Return:
> + * * true - Preferred locality set
> + * * false - Invalid locality specified
> + */
> +bool tpm_chip_preferred_locality(struct tpm_chip *chip, int locality)
> +{
> + if (locality < 0 || locality >=TPM_MAX_LOCALITY)
> + return false;
> +
> + mutex_lock(>tpm_mutex);
> + chip->pref_locality = locality;
> + mutex_unlock(>tpm_mutex);
> + return true;
> +}
> +EXPORT_SYMBOL_GPL(tpm_chip_preferred_locality);
> +
> /**
> * tpm_try_get_ops() - Get a ref to the tpm_chip
> * @chip: Chip to ref
> @@ -374,6 +395,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
> }
>
> chip->locality = -1;
> + chip->pref_locality = 0;
> return chip;
>
> out:
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 5da134f12c9a..35f14ccecf0e 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -274,6 +274,21 @@ int tpm_is_tpm2(struct tpm_chip *chip)
> }
> EXPORT_SYMBOL_GPL(tpm_is_tpm2);
>
> +/**
> + * tpm_preferred_locality() - set the TPM chip preferred locality to open
> + * @chip:a TPM chip to use
> + * @locality: the preferred locality
> + *
> + * Return:
> + * * true - Preferred locality set
> + * * false - Invalid locality specified
> + */
> +bool tpm_preferred_locality(struct tpm_chip *chip, int locality)
> +{
> + return tpm_chip_preferred_locality(chip, locality);
> +}
> +EXPORT_SYMBOL_GPL(tpm_preferred_locality);
What good does this extra wrapping do?
tpm_set_default_locality() and default_locality would make so much more
sense in any case.
BR, Jarkko
Re: [PATCH v9 14/19] tpm: Ensure tpm is in known state at startup
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> From: "Daniel P. Smith"
>
> When tis core initializes, it assumes all localities are closed. There
s/tis_core/tpm_tis_core/
> are cases when this may not be the case. This commit addresses this by
> ensuring all localities are closed before initializing begins.
>
> Signed-off-by: Daniel P. Smith
> Signed-off-by: Ross Philipson
> ---
> drivers/char/tpm/tpm_tis_core.c | 11 ++-
> include/linux/tpm.h | 6 ++
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 7c1761bd6000..9fb53bb3e73f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1104,7 +1104,7 @@ int tpm_tis_core_init(struct device *dev, struct
> tpm_tis_data *priv, int irq,
> u32 intmask;
> u32 clkrun_val;
> u8 rid;
> - int rc, probe;
> + int rc, probe, i;
> struct tpm_chip *chip;
>
> chip = tpmm_chip_alloc(dev, _tis);
> @@ -1166,6 +1166,15 @@ int tpm_tis_core_init(struct device *dev, struct
> tpm_tis_data *priv, int irq,
> goto out_err;
> }
>
> + /*
> + * There are environments, like Intel TXT, that may leave a TPM
What else at this point than Intel TXT reflecting the state of the
mainline?
> + * locality open. Close all localities to start from a known state.
> + */
> + for (i = 0; i <= TPM_MAX_LOCALITY; i++) {
> + if (check_locality(chip, i))
> + tpm_tis_relinquish_locality(chip, i);
> + }
To be strict this should be enabled only for x86 platforms.
I.e. should be flagged.
> +
> /* Take control of the TPM's interrupt hardware and shut it off */
> rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), );
> if (rc < 0)
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index c17e4efbb2e5..363f7078c3a9 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -147,6 +147,12 @@ struct tpm_chip_seqops {
> */
> #define TPM2_MAX_CONTEXT_SIZE 4096
>
> +/*
> + * The maximum locality (0 - 4) for a TPM, as defined in section 3.2 of the
> + * Client Platform Profile Specification.
> + */
> +#define TPM_MAX_LOCALITY 4
> +
> struct tpm_chip {
> struct device dev;
> struct device devs;
BR, Jarkko
Re: [PATCH v9 13/19] tpm: Protect against locality counter underflow
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> From: "Daniel P. Smith"
>
> Commit 933bfc5ad213 introduced the use of a locality counter to control when a
> locality request is allowed to be sent to the TPM. In the commit, the counter
> is indiscriminately decremented. Thus creating a situation for an integer
> underflow of the counter.
>
> Signed-off-by: Daniel P. Smith
> Signed-off-by: Ross Philipson
> Reported-by: Kanth Ghatraju
> Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
Not sure if we have practical use for fixes tag here but open for
argument ofc. I.e. I'm not sure what is the practical scenario to
worry about if Trenchboot did not exist.
> ---
> drivers/char/tpm/tpm_tis_core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 176cd8dbf1db..7c1761bd6000 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -180,7 +180,8 @@ static int tpm_tis_relinquish_locality(struct tpm_chip
> *chip, int l)
> struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>
> mutex_lock(>locality_count_mutex);
> - priv->locality_count--;
> + if (priv->locality_count > 0)
> + priv->locality_count--;
I'd signal the situation with pr_info() in else branch.
> if (priv->locality_count == 0)
> __tpm_tis_relinquish_locality(priv, l);
> mutex_unlock(>locality_count_mutex);
BR, Jarkko
Re: [PATCH v9 10/19] x86: Secure Launch SMP bringup support
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> On Intel, the APs are left in a well documented state after TXT performs
> the late launch. Specifically they cannot have #INIT asserted on them so
> a standard startup via INIT/SIPI/SIPI cannot be performed. Instead the
> early SL stub code uses MONITOR and MWAIT to park the APs. The realmode/init.c
> code updates the jump address for the waiting APs with the location of the
> Secure Launch entry point in the RM piggy after it is loaded and fixed up.
> As the APs are woken up by writing the monitor, the APs jump to the Secure
> Launch entry point in the RM piggy which mimics what the real mode code would
> do then jumps to the standard RM piggy protected mode entry point.
>
> Signed-off-by: Ross Philipson
> ---
> arch/x86/include/asm/realmode.h | 3 ++
> arch/x86/kernel/smpboot.c| 58 +++-
> arch/x86/realmode/init.c | 3 ++
> arch/x86/realmode/rm/header.S| 3 ++
> arch/x86/realmode/rm/trampoline_64.S | 32 +++
> 5 files changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
> index 87e5482acd0d..339b48e2543d 100644
> --- a/arch/x86/include/asm/realmode.h
> +++ b/arch/x86/include/asm/realmode.h
> @@ -38,6 +38,9 @@ struct real_mode_header {
> #ifdef CONFIG_X86_64
> u32 machine_real_restart_seg;
> #endif
> +#ifdef CONFIG_SECURE_LAUNCH
> + u32 sl_trampoline_start32;
> +#endif
> };
>
> /* This must match data at realmode/rm/trampoline_{32,64}.S */
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 0c35207320cb..adb521221d6c 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -60,6 +60,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -868,6 +869,56 @@ int common_cpu_up(unsigned int cpu, struct task_struct
> *idle)
> return 0;
> }
>
> +#ifdef CONFIG_SECURE_LAUNCH
> +
> +static bool slaunch_is_txt_launch(void)
> +{
> + if ((slaunch_get_flags() & (SL_FLAG_ACTIVE|SL_FLAG_ARCH_TXT)) ==
> + (SL_FLAG_ACTIVE | SL_FLAG_ARCH_TXT))
> + return true;
> +
> + return false;
> +}
static inline bool slaunch_is_txt_launch(void)
{
u32 mask = SL_FLAG_ACTIVE | SL_FLAG_ARCH_TXT;
return slaunch_get_flags() & mask == mask;
}
> +
> +/*
> + * TXT AP startup is quite different than normal. The APs cannot have #INIT
> + * asserted on them or receive SIPIs. The early Secure Launch code has parked
> + * the APs using monitor/mwait. This will wake the APs by writing the monitor
> + * and have them jump to the protected mode code in the rmpiggy where the
> rest
> + * of the SMP boot of the AP will proceed normally.
> + */
> +static void slaunch_wakeup_cpu_from_txt(int cpu, int apicid)
> +{
> + struct sl_ap_wake_info *ap_wake_info;
> + struct sl_ap_stack_and_monitor *stack_monitor = NULL;
struct sl_ap_stack_and_monitor *stack_monitor; /* note: no initialization */
struct sl_ap_wake_info *ap_wake_info;
> +
> + ap_wake_info = slaunch_get_ap_wake_info();
> +
> + stack_monitor = (struct sl_ap_stack_and_monitor
> *)__va(ap_wake_info->ap_wake_block +
> +
> ap_wake_info->ap_stacks_offset);
> +
> + for (unsigned int i = TXT_MAX_CPUS - 1; i >= 0; i--) {
> + if (stack_monitor[i].apicid == apicid) {
> + /* Write the monitor */
I'd remove this comment.
> + stack_monitor[i].monitor = 1;
> + break;
> + }
> + }
> +}
> +
> +#else
> +
> +static inline bool slaunch_is_txt_launch(void)
> +{
> + return false;
> +}
> +
> +static inline void slaunch_wakeup_cpu_from_txt(int cpu, int apicid)
> +{
> +}
> +
> +#endif /* !CONFIG_SECURE_LAUNCH */
> +
> /*
> * NOTE - on most systems this is a PHYSICAL apic ID, but on multiquad
> * (ie clustered apic addressing mode), this is a LOGICAL apic ID.
> @@ -877,7 +928,7 @@ int common_cpu_up(unsigned int cpu, struct task_struct
> *idle)
> static int do_boot_cpu(u32 apicid, int cpu, struct task_struct *idle)
> {
> unsigned long start_ip = real_mode_header->trampoline_start;
> - int ret;
> + int ret = 0;
>
> #ifdef CONFIG_X86_64
> /* If 64-bit wakeup method exists, use the 64-bit mode trampoline IP */
> @@ -922,12 +973,15 @@ static int do_boot_cpu(u32 apicid, int cpu, struct
> task_struct *idle)
>
> /*
>* Wake up a CPU in difference cases:
> + * - Intel TXT DRTM launch uses its own method to wake the APs
>* - Use a method from the APIC driver if one defined, with wakeup
>* straight to 64-bit mode preferred over wakeup to RM.
>* Otherwise,
>* - Use an INIT boot APIC message
>*/
> - if (apic->wakeup_secondary_cpu_64)
> + if (slaunch_is_txt_launch())
> +
Re: [PATCH v9 09/19] x86: Secure Launch kernel late boot stub
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > The routine slaunch_setup is called out of the x86 specific setup_arch() > routine during early kernel boot. After determining what platform is > present, various operations specific to that platform occur. This > includes finalizing setting for the platform late launch and verifying > that memory protections are in place. > > For TXT, this code also reserves the original compressed kernel setup > area where the APs were left looping so that this memory cannot be used. > > Signed-off-by: Ross Philipson > --- > arch/x86/kernel/Makefile | 1 + > arch/x86/kernel/setup.c| 3 + > arch/x86/kernel/slaunch.c | 525 + > drivers/iommu/intel/dmar.c | 4 + > 4 files changed, 533 insertions(+) > create mode 100644 arch/x86/kernel/slaunch.c > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index 5d128167e2e2..b35ca99ab0a0 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -76,6 +76,7 @@ obj-$(CONFIG_X86_32)+= tls.o > obj-$(CONFIG_IA32_EMULATION) += tls.o > obj-y+= step.o > obj-$(CONFIG_INTEL_TXT) += tboot.o > +obj-$(CONFIG_SECURE_LAUNCH) += slaunch.o Hmm... should that be CONFIG_X86_SECURE_LAUNCH? Just asking... BR, Jarkko
Re: [PATCH v9 09/19] x86: Secure Launch kernel late boot stub
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > The routine slaunch_setup is called out of the x86 specific setup_arch() > routine during early kernel boot. After determining what platform is > present, various operations specific to that platform occur. This > includes finalizing setting for the platform late launch and verifying > that memory protections are in place. "memory protections" is not too helpful tbh. Better to describe very briefly the VT-d usage. BR, Jarkko
Re: [PATCH v9 08/19] x86: Secure Launch kernel early boot stub
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > The Secure Launch (SL) stub provides the entry point for Intel TXT (and > later AMD SKINIT) to vector to during the late launch. The symbol > sl_stub_entry is that entry point and its offset into the kernel is > conveyed to the launching code using the MLE (Measured Launch > Environment) header in the structure named mle_header. The offset of the > MLE header is set in the kernel_info. The routine sl_stub contains the > very early late launch setup code responsible for setting up the basic > environment to allow the normal kernel startup_32 code to proceed. It is > also responsible for properly waking and handling the APs on Intel > platforms. The routine sl_main which runs after entering 64b mode is > responsible for measuring configuration and module information before > it is used like the boot params, the kernel command line, the TXT heap, > an external initramfs, etc. > > Signed-off-by: Ross Philipson > --- > Documentation/arch/x86/boot.rst| 21 + > arch/x86/boot/compressed/Makefile | 3 +- > arch/x86/boot/compressed/head_64.S | 30 + > arch/x86/boot/compressed/kernel_info.S | 34 ++ > arch/x86/boot/compressed/sl_main.c | 577 > arch/x86/boot/compressed/sl_stub.S | 725 + > arch/x86/include/asm/msr-index.h | 5 + > arch/x86/include/uapi/asm/bootparam.h | 1 + > arch/x86/kernel/asm-offsets.c | 20 + > 9 files changed, 1415 insertions(+), 1 deletion(-) > create mode 100644 arch/x86/boot/compressed/sl_main.c > create mode 100644 arch/x86/boot/compressed/sl_stub.S > > diff --git a/Documentation/arch/x86/boot.rst b/Documentation/arch/x86/boot.rst > index 4fd492cb4970..295cdf9bcbdb 100644 > --- a/Documentation/arch/x86/boot.rst > +++ b/Documentation/arch/x86/boot.rst > @@ -482,6 +482,14 @@ Protocol:2.00+ > - If 1, KASLR enabled. > - If 0, KASLR disabled. > > + Bit 2 (kernel internal): SLAUNCH_FLAG > + > + - Used internally by the setup kernel to communicate > + Secure Launch status to kernel proper. > + > + - If 1, Secure Launch enabled. > + - If 0, Secure Launch disabled. > + >Bit 5 (write): QUIET_FLAG > > - If 0, print early messages. > @@ -1028,6 +1036,19 @@ Offset/size: 0x000c/4 > >This field contains maximal allowed type for setup_data and setup_indirect > structs. > > + = > +Field name: mle_header_offset > +Offset/size: 0x0010/4 > + = > + > + This field contains the offset to the Secure Launch Measured Launch > Environment > + (MLE) header. This offset is used to locate information needed during a > secure > + late launch using Intel TXT. If the offset is zero, the kernel does not > have > + Secure Launch capabilities. The MLE entry point is called from TXT on the > BSP > + following a success measured launch. The specific state of the processors > is > + outlined in the TXT Software Development Guide, the latest can be found > here: > + > https://www.intel.com/content/dam/www/public/us/en/documents/guides/intel-txt-software-development-guide.pdf > + > > The Image Checksum > == > diff --git a/arch/x86/boot/compressed/Makefile > b/arch/x86/boot/compressed/Makefile > index 9189a0e28686..9076a248d4b4 100644 > --- a/arch/x86/boot/compressed/Makefile > +++ b/arch/x86/boot/compressed/Makefile > @@ -118,7 +118,8 @@ vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o > vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o > vmlinux-objs-$(CONFIG_EFI_STUB) += > $(objtree)/drivers/firmware/efi/libstub/lib.a > > -vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o > $(obj)/early_sha256.o > +vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o > $(obj)/early_sha256.o \ > + $(obj)/sl_main.o $(obj)/sl_stub.o > > $(obj)/vmlinux: $(vmlinux-objs-y) FORCE > $(call if_changed,ld) > diff --git a/arch/x86/boot/compressed/head_64.S > b/arch/x86/boot/compressed/head_64.S > index 1dcb794c5479..803c9e2e6d85 100644 > --- a/arch/x86/boot/compressed/head_64.S > +++ b/arch/x86/boot/compressed/head_64.S > @@ -420,6 +420,13 @@ SYM_CODE_START(startup_64) > pushq $0 > popfq > > +#ifdef CONFIG_SECURE_LAUNCH > + /* Ensure the relocation region is coverd by a PMR */ > + movq%rbx, %rdi > + movl$(_bss - startup_32), %esi > + callq sl_check_region > +#endif > + > /* > * Copy the compressed kernel to the end of our buffer > * where decompression in place becomes safe. > @@ -462,6 +469,29 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated) > shrq$3, %rcx > rep stosq > > +#ifdef CONFIG_SECURE_LAUNCH > + /* > + * Have to do the final early sl stub work in 64b area. > + * > + * *** NOTE *** > + * > + * Several boot params get used before we get a chance to measure > + * them in
Re: [PATCH v9 06/19] x86: Add early SHA-1 support for Secure Launch early measurements
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> From: "Daniel P. Smith"
>
> For better or worse, Secure Launch needs SHA-1 and SHA-256. The
> choice of hashes used lie with the platform firmware, not with
> software, and is often outside of the users control.
>
> Even if we'd prefer to use SHA-256-only, if firmware elected to start us
> with the SHA-1 and SHA-256 backs active, we still need SHA-1 to parse
> the TPM event log thus far, and deliberately cap the SHA-1 PCRs in order
> to safely use SHA-256 for everything else.
>
> The SHA-1 code here has its origins in the code from the main kernel:
>
> commit c4d5b9ffa31f ("crypto: sha1 - implement base layer for SHA-1")
>
> A modified version of this code was introduced to the lib/crypto/sha1.c
> to bring it in line with the SHA-256 code and allow it to be pulled into the
> setup kernel in the same manner as SHA-256 is.
>
> Signed-off-by: Daniel P. Smith
> Signed-off-by: Ross Philipson
> ---
> arch/x86/boot/compressed/Makefile | 2 +
> arch/x86/boot/compressed/early_sha1.c | 12
> include/crypto/sha1.h | 1 +
> lib/crypto/sha1.c | 81 +++
> 4 files changed, 96 insertions(+)
> create mode 100644 arch/x86/boot/compressed/early_sha1.c
>
> diff --git a/arch/x86/boot/compressed/Makefile
> b/arch/x86/boot/compressed/Makefile
> index e9522c6893be..3307ebef4e1b 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -118,6 +118,8 @@ vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
> vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
> vmlinux-objs-$(CONFIG_EFI_STUB) +=
> $(objtree)/drivers/firmware/efi/libstub/lib.a
>
> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o
> +
> $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
> $(call if_changed,ld)
>
> diff --git a/arch/x86/boot/compressed/early_sha1.c
> b/arch/x86/boot/compressed/early_sha1.c
> new file mode 100644
> index ..8a9b904a73ab
> --- /dev/null
> +++ b/arch/x86/boot/compressed/early_sha1.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Apertus Solutions, LLC.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "../../../../lib/crypto/sha1.c"
}
Yep, make sense. Thinking only that should this be just sha1.c.
Comparing this to mainly drivers/firmware/efi/tpm.c, which is not
early_tpm.c where the early actually probably would make more sense
than here. Here sha1 primitive is just needed.
This is definitely a nitpick but why carry a prefix that is not
that useful, right?
> diff --git a/include/crypto/sha1.h b/include/crypto/sha1.h
> index 044ecea60ac8..d715dd5332e1 100644
> --- a/include/crypto/sha1.h
> +++ b/include/crypto/sha1.h
> @@ -42,5 +42,6 @@ extern int crypto_sha1_finup(struct shash_desc *desc, const
> u8 *data,
> #define SHA1_WORKSPACE_WORDS 16
> void sha1_init(__u32 *buf);
> void sha1_transform(__u32 *digest, const char *data, __u32 *W);
> +void sha1(const u8 *data, unsigned int len, u8 *out);
>
> #endif /* _CRYPTO_SHA1_H */
> diff --git a/lib/crypto/sha1.c b/lib/crypto/sha1.c
> index 1aebe7be9401..10152125b338 100644
> --- a/lib/crypto/sha1.c
> +++ b/lib/crypto/sha1.c
> @@ -137,4 +137,85 @@ void sha1_init(__u32 *buf)
> }
> EXPORT_SYMBOL(sha1_init);
>
> +static void __sha1_transform(u32 *digest, const char *data)
> +{
> + u32 ws[SHA1_WORKSPACE_WORDS];
> +
> + sha1_transform(digest, data, ws);
> +
> + memzero_explicit(ws, sizeof(ws));
For the sake of future reference I'd carry always some inline comment
with any memzero_explicit() call site.
> +}
> +
> +static void sha1_update(struct sha1_state *sctx, const u8 *data, unsigned
> int len)
> +{
> + unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
> +
> + sctx->count += len;
> +
> + if (likely((partial + len) >= SHA1_BLOCK_SIZE)) {
if (unlikely((partial + len) < SHA1_BLOCK_SIZE))
goto out;
?
> + int blocks;
> +
> + if (partial) {
> + int p = SHA1_BLOCK_SIZE - partial;
> +
> + memcpy(sctx->buffer + partial, data, p);
> + data += p;
> + len -= p;
> +
> + __sha1_transform(sctx->state, sctx->buffer);
> + }
> +
> + blocks = len / SHA1_BLOCK_SIZE;
> + len %= SHA1_BLOCK_SIZE;
> +
> + if (blocks) {
> + while (blocks--) {
> + __sha1_transform(sctx->state, data);
> + data += SHA1_BLOCK_SIZE;
> + }
> + }
> + partial = 0;
> + }
> +
out:
> + if (len)
> + memcpy(sctx->buffer + partial, data, len);
Why not just memcpy() unconditionally?
> +}
> +
> +static void sha1_final(struct sha1_state *sctx, u8 *out)
> +{
> + const int bit_offset =
Re: [PATCH v9 05/19] x86: Secure Launch main header file
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > Introduce the main Secure Launch header file used in the early SL stub > and the early setup code. > > Signed-off-by: Ross Philipson Right and anything AMD specific should also have legit references. I actually always compare to the spec when I review, so not just nitpicking really. I'm actually bit confused, is this usable both Intel and AMD in the current state? Might be just that have not had time follow this for some time. BR, Jarkko
Re: [PATCH v9 04/19] x86: Secure Launch Resource Table header file
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > Introduce the Secure Launch Resource Table which forms the formal > interface between the pre and post launch code. > > Signed-off-by: Ross Philipson If a uarch specific, I'd appreciate Intel SDM reference here so that I can look it up and compare. Like in section granularity. BR, Jarkko
Re: [PATCH v9 01/19] x86/boot: Place kernel_info at a fixed offset
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> From: Arvind Sankar
>
> There are use cases for storing the offset of a symbol in kernel_info.
> For example, the trenchboot series [0] needs to store the offset of the
> Measured Launch Environment header in kernel_info.
So either there are other use cases that you should enumerate, or just
be straight and state that this is done for Trenchboot.
I believe latter is the case, and there is no reason to project further.
If it does not interfere kernel otherwise, it should be fine just by
that.
Also I believe that it is written as Trenchboot, without "series" ;-)
Think when writing commit message that it will some day be part of the
commit log, not a series flying in the air.
Sorry for the nitpicks but better to be punctual and that way also
transparent as possible, right?
>
> Since commit (note: commit ID from tip/master)
>
> commit 527afc212231 ("x86/boot: Check that there are no run-time relocations")
>
> run-time relocations are not allowed in the compressed kernel, so simply
> using the symbol in kernel_info, as
>
> .long symbol
>
> will cause a linker error because this is not position-independent.
>
> With kernel_info being a separate object file and in a different section
> from startup_32, there is no way to calculate the offset of a symbol
> from the start of the image in a position-independent way.
>
> To enable such use cases, put kernel_info into its own section which is
"To allow Trenchboot to access the fields of kernel_info..."
Much more understandable.
> placed at a predetermined offset (KERNEL_INFO_OFFSET) via the linker
> script. This will allow calculating the symbol offset in a
> position-independent way, by adding the offset from the start of
> kernel_info to KERNEL_INFO_OFFSET.
>
> Ensure that kernel_info is aligned, and use the SYM_DATA.* macros
> instead of bare labels. This stores the size of the kernel_info
> structure in the ELF symbol table.
Aligned to which boundary and short explanation why to that boundary,
i.e. state the obvious if you bring it up anyway here.
Just seems to be progressing pretty well so taking my eye glass and
looking into nitty gritty details...
BR, Jarkko
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
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 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: [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 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 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 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: 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
