| Message ID | 20240531010331.134441-7-ross.philipson@oracle.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Trenchboot secure dynamic launch Linux kernel support | expand |
Eric Biggers <ebiggers@kernel.org> writes: > On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> 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 <dpsmith@apertussolutions.com> >> Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > > Thanks. This explanation doesn't seem to have made it into the actual code or > documentation. Can you please get it into a more permanent location? > > Also, can you point to where the "deliberately cap the SHA-1 PCRs" thing happens > in the code? > > That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use > SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is > that the case? Sure, maybe there are situations where you *have* to use SHA-1, > but why would you not at least *prefer* SHA-256? Yes. Please prefer to use SHA-256. Have you considered implementing I think it is SHA1-DC (as git has) that is compatible with SHA1 but blocks the known class of attacks where sha1 is actively broken at this point? No offense to your Trenchboot project but my gut says that anything new relying on SHA-1 is probably a bad joke at this point. Firmware can most definitely be upgraded and if the goal is a more secure boot the usual backwards compatibility concerns for supporting old firmware really don't apply. Perhaps hide all of the SHA-1 stuff behind CONFIG_TRENCHBOOT_PROTOTYPE or something like that to make it clear that SHA-1 is not appropriate for any kind of production deployment and is only good for prototyping your implementation until properly implemented firmware comes along. Eric
On 5/30/24 7:16 PM, Eric Biggers wrote: > On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> 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 <dpsmith@apertussolutions.com> >> Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > > Thanks. This explanation doesn't seem to have made it into the actual code or > documentation. Can you please get it into a more permanent location? > > Also, can you point to where the "deliberately cap the SHA-1 PCRs" thing happens > in the code? > > That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use > SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is > that the case? Sure, maybe there are situations where you *have* to use SHA-1, > but why would you not at least *prefer* SHA-256? Yes those are fair points. We will address them and indicate we prefer SHA-256 or better. > >> /* >> * An implementation of SHA-1's compression function. Don't use in new code! >> * You shouldn't be using SHA-1, and even if you *have* to use SHA-1, this isn't >> * the correct way to hash something with SHA-1 (use crypto_shash instead). >> */ >> #define SHA1_DIGEST_WORDS (SHA1_DIGEST_SIZE / 4) >> #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); > > Also, the comment above needs to be updated. Ack, will address. Thank you > > - Eric
On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > 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 <dpsmith@apertussolutions.com> > Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > --- > 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 000000000000..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 <linux/init.h> > +#include <linux/linkage.h> > +#include <linux/string.h> > +#include <asm/boot.h> > +#include <asm/unaligned.h> > + > +#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 = SHA1_BLOCK_SIZE - sizeof(__be64); > + unsigned int partial = sctx->count % SHA1_BLOCK_SIZE; > + __be64 *bits = (__be64 *)(sctx->buffer + bit_offset); > + __be32 *digest = (__be32 *)out; > + int i; > + > + sctx->buffer[partial++] = 0x80; > + if (partial > bit_offset) { > + memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial); > + partial = 0; > + > + __sha1_transform(sctx->state, sctx->buffer); > + } > + > + memset(sctx->buffer + partial, 0x0, bit_offset - partial); > + *bits = cpu_to_be64(sctx->count << 3); > + __sha1_transform(sctx->state, sctx->buffer); > + > + for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++) > + put_unaligned_be32(sctx->state[i], digest++); > + > + *sctx = (struct sha1_state){}; > +} > + > +void sha1(const u8 *data, unsigned int len, u8 *out) > +{ > + struct sha1_state sctx = {0}; > + > + sha1_init(sctx.state); > + sctx.count = 0; Hmm... so shouldn't C99 take care of this given the initialization above? I'm not 100% sure here. I.e. given "= {0}", shouldn't this already be zero? > + sha1_update(&sctx, data, len); > + sha1_final(&sctx, out); > +} > +EXPORT_SYMBOL(sha1); > + > MODULE_LICENSE("GPL"); BR, Jarkko
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" <dpsmith@apertussolutions.com> >> >> 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 <dpsmith@apertussolutions.com> >> Signed-off-by: Ross Philipson <ross.philipson@oracle.com> >> --- >> 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 000000000000..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 <linux/init.h> >> +#include <linux/linkage.h> >> +#include <linux/string.h> >> +#include <asm/boot.h> >> +#include <asm/unaligned.h> >> + >> +#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. > >> 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. We can do that. > >> +} >> + >> +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; > > ? We could do it that way. I guess it would cut down in indenting. I defer to Daniel Smith on this... > >> + 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? > ... and this. >> +} >> + >> +static void sha1_final(struct sha1_state *sctx, u8 *out) >> +{ >> + const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64); >> + unsigned int partial = sctx->count % SHA1_BLOCK_SIZE; >> + __be64 *bits = (__be64 *)(sctx->buffer + bit_offset); >> + __be32 *digest = (__be32 *)out; >> + int i; >> + >> + sctx->buffer[partial++] = 0x80; >> + if (partial > bit_offset) { >> + memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial); >> + partial = 0; >> + >> + __sha1_transform(sctx->state, sctx->buffer); >> + } >> + >> + memset(sctx->buffer + partial, 0x0, bit_offset - partial); >> + *bits = cpu_to_be64(sctx->count << 3); >> + __sha1_transform(sctx->state, sctx->buffer); >> + >> + for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++) >> + put_unaligned_be32(sctx->state[i], digest++); >> + >> + *sctx = (struct sha1_state){}; >> +} >> + >> +void sha1(const u8 *data, unsigned int len, u8 *out) >> +{ >> + struct sha1_state sctx = {0}; >> + >> + sha1_init(sctx.state); >> + sctx.count = 0; > > Hmm... so shouldn't C99 take care of this given the initialization > above? I'm not 100% sure here. I.e. given "= {0}", shouldn't this > already be zero? Yes it seems so. We will look at changing that. > >> + sha1_update(&sctx, data, len); >> + sha1_final(&sctx, out); >> +} >> +EXPORT_SYMBOL(sha1); >> + >> MODULE_LICENSE("GPL"); > > BR, Jarkko Thanks Ross
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" <dpsmith@apertussolutions.com> > >> > >> 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 <dpsmith@apertussolutions.com> > >> Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > >> --- > >> 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 000000000000..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 <linux/init.h> > >> +#include <linux/linkage.h> > >> +#include <linux/string.h> > >> +#include <asm/boot.h> > >> +#include <asm/unaligned.h> > >> + > >> +#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) > >> +{ > >> + 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. > > We can do that. > > > > >> +} > >> + > >> +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; > > > > ? > > We could do it that way. I guess it would cut down in indenting. I defer > to Daniel Smith on this... Yep, that's why I requested this. > > > > >> + 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? > > > > ... and this. It only adds complexity with no gain. > > >> +} > >> + > >> +static void sha1_final(struct sha1_state *sctx, u8 *out) > >> +{ > >> + const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64); > >> + unsigned int partial = sctx->count % SHA1_BLOCK_SIZE; > >> + __be64 *bits = (__be64 *)(sctx->buffer + bit_offset); > >> + __be32 *digest = (__be32 *)out; > >> + int i; > >> + > >> + sctx->buffer[partial++] = 0x80; > >> + if (partial > bit_offset) { > >> + memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial); > >> + partial = 0; > >> + > >> + __sha1_transform(sctx->state, sctx->buffer); > >> + } > >> + > >> + memset(sctx->buffer + partial, 0x0, bit_offset - partial); > >> + *bits = cpu_to_be64(sctx->count << 3); > >> + __sha1_transform(sctx->state, sctx->buffer); > >> + > >> + for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++) > >> + put_unaligned_be32(sctx->state[i], digest++); > >> + > >> + *sctx = (struct sha1_state){}; > >> +} > >> + > >> +void sha1(const u8 *data, unsigned int len, u8 *out) > >> +{ > >> + struct sha1_state sctx = {0}; > >> + > >> + sha1_init(sctx.state); > >> + sctx.count = 0; > > > > Hmm... so shouldn't C99 take care of this given the initialization > > above? I'm not 100% sure here. I.e. given "= {0}", shouldn't this > > already be zero? > > Yes it seems so. We will look at changing that. Yeah, AFAIK C99 should zero out anything remaining. > > > > >> + sha1_update(&sctx, data, len); > >> + sha1_final(&sctx, out); > >> +} > >> +EXPORT_SYMBOL(sha1); > >> + > >> MODULE_LICENSE("GPL"); > > > > BR, Jarkko > > Thanks > Ross BR, Jarkko
On 5/31/24 09:54, Eric W. Biederman wrote: > Eric Biggers <ebiggers@kernel.org> writes: > >> On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> 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 <dpsmith@apertussolutions.com> >>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com> >> >> Thanks. This explanation doesn't seem to have made it into the actual code or >> documentation. Can you please get it into a more permanent location? >> >> Also, can you point to where the "deliberately cap the SHA-1 PCRs" thing happens >> in the code? >> >> That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use >> SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is >> that the case? Sure, maybe there are situations where you *have* to use SHA-1, >> but why would you not at least *prefer* SHA-256? > > Yes. Please prefer to use SHA-256. > > Have you considered implementing I think it is SHA1-DC (as git has) that > is compatible with SHA1 but blocks the known class of attacks where > sha1 is actively broken at this point? We are using the kernel's implementation, addressing what the kernel provides is beyond our efforts. Perhaps someone who is interested in improving the kernel's SHA1 could submit a patch implementing/replacing it with SHA1-DC, as I am sure the maintainers would welcome the help. v/r, dps
On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: > On 5/31/24 09:54, Eric W. Biederman wrote: >> Eric Biggers <ebiggers@kernel.org> writes: >>> That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use >>> SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is >>> that the case? Sure, maybe there are situations where you *have* to use SHA-1, >>> but why would you not at least *prefer* SHA-256? >> >> Yes. Please prefer to use SHA-256. >> >> Have you considered implementing I think it is SHA1-DC (as git has) that >> is compatible with SHA1 but blocks the known class of attacks where >> sha1 is actively broken at this point? > > We are using the kernel's implementation, addressing what the kernel > provides is beyond our efforts. Perhaps someone who is interested in > improving the kernel's SHA1 could submit a patch implementing/replacing > it with SHA1-DC, as I am sure the maintainers would welcome the help. Well, someone who is interested to get his "secure" code merged should have a vested interested to have a non-broken SHA1 implementation if there is a sensible requirement to use SHA1 in that new "secure" code, no? Just for the record. The related maintainers can rightfully decide to reject known broken "secure" code on a purely technical argument. Thanks, tglx
On Thu Aug 15, 2024 at 10:10 PM EEST, Thomas Gleixner wrote: > On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: > > On 5/31/24 09:54, Eric W. Biederman wrote: > >> Eric Biggers <ebiggers@kernel.org> writes: > >>> That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use > >>> SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is > >>> that the case? Sure, maybe there are situations where you *have* to use SHA-1, > >>> but why would you not at least *prefer* SHA-256? > >> > >> Yes. Please prefer to use SHA-256. > >> > >> Have you considered implementing I think it is SHA1-DC (as git has) that > >> is compatible with SHA1 but blocks the known class of attacks where > >> sha1 is actively broken at this point? > > > > We are using the kernel's implementation, addressing what the kernel > > provides is beyond our efforts. Perhaps someone who is interested in > > improving the kernel's SHA1 could submit a patch implementing/replacing > > it with SHA1-DC, as I am sure the maintainers would welcome the help. Git also has a bit more wide than secure launch, and the timeline is also completely different. Git maintains legacy, while has also introduced SHA-256 support in 2018. This as a new feature in the kernel stack. The purpose of SHA1-DC has obviously been to extend the lifespan, not fix SHA-1. Linux will be better of not adding anything new related to SHA-1 or TPM 1.2. They still have a maintenance cost and I think that time would be better spent of for almost anything else (starting from taking your trashes out or boiling coffee) ;-) > > Well, someone who is interested to get his "secure" code merged should > have a vested interested to have a non-broken SHA1 implementation if > there is a sensible requirement to use SHA1 in that new "secure" code, > no? > > Just for the record. The related maintainers can rightfully decide to > reject known broken "secure" code on a purely technical argument. > > Thanks, > > tglx BR, Jarkko
On 15/08/2024 8:10 pm, Thomas Gleixner wrote: > On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: >> On 5/31/24 09:54, Eric W. Biederman wrote: >>> Eric Biggers <ebiggers@kernel.org> writes: >>>> That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use >>>> SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is >>>> that the case? Sure, maybe there are situations where you *have* to use SHA-1, >>>> but why would you not at least *prefer* SHA-256? >>> Yes. Please prefer to use SHA-256. >>> >>> Have you considered implementing I think it is SHA1-DC (as git has) that >>> is compatible with SHA1 but blocks the known class of attacks where >>> sha1 is actively broken at this point? >> We are using the kernel's implementation, addressing what the kernel >> provides is beyond our efforts. Perhaps someone who is interested in >> improving the kernel's SHA1 could submit a patch implementing/replacing >> it with SHA1-DC, as I am sure the maintainers would welcome the help. > Well, someone who is interested to get his "secure" code merged should > have a vested interested to have a non-broken SHA1 implementation if > there is a sensible requirement to use SHA1 in that new "secure" code, > no? No. The use of SHA-1 is necessary even on modern systems to avoid a vulnerability. It is the platform, not Linux, which decides which TPM PCR banks are active. Linux *must* have an algorithm for every active bank (which is the platform's choice), even if the single thing it intends to do is cap the bank and use better ones. Capping a bank requires updating the TPM Log without corrupting it, which requires a hash calculation of the correct type for the bank. ~Andrew
On Fri Aug 16, 2024 at 2:01 PM EEST, Andrew Cooper wrote: > On 15/08/2024 8:10 pm, Thomas Gleixner wrote: > > On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: > >> On 5/31/24 09:54, Eric W. Biederman wrote: > >>> Eric Biggers <ebiggers@kernel.org> writes: > >>>> That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use > >>>> SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is > >>>> that the case? Sure, maybe there are situations where you *have* to use SHA-1, > >>>> but why would you not at least *prefer* SHA-256? > >>> Yes. Please prefer to use SHA-256. > >>> > >>> Have you considered implementing I think it is SHA1-DC (as git has) that > >>> is compatible with SHA1 but blocks the known class of attacks where > >>> sha1 is actively broken at this point? > >> We are using the kernel's implementation, addressing what the kernel > >> provides is beyond our efforts. Perhaps someone who is interested in > >> improving the kernel's SHA1 could submit a patch implementing/replacing > >> it with SHA1-DC, as I am sure the maintainers would welcome the help. > > Well, someone who is interested to get his "secure" code merged should > > have a vested interested to have a non-broken SHA1 implementation if > > there is a sensible requirement to use SHA1 in that new "secure" code, > > no? > > No. > > The use of SHA-1 is necessary even on modern systems to avoid a > vulnerability. > > It is the platform, not Linux, which decides which TPM PCR banks are active. > > Linux *must* have an algorithm for every active bank (which is the > platform's choice), even if the single thing it intends to do is cap the > bank and use better ones. For (any) non-legacy features we can choose, which choices we choose to support, and which we do not. This is not an oppositive view just saying how it is, and platforms set of choices is not a selling argument. > > Capping a bank requires updating the TPM Log without corrupting it, > which requires a hash calculation of the correct type for the bank. > > ~Andrew BR, Jarkko
On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote: > For (any) non-legacy features we can choose, which choices we choose to > support, and which we do not. This is not an oppositive view just saying > how it is, and platforms set of choices is not a selling argument. NIST still permits the use of SHA-1 until 2030, and the most significant demonstrated weaknesses in it don't seem applicable to the use case here. We certainly shouldn't encourage any new uses of it, and anyone who's able to use SHA-2 should be doing that instead, but it feels like people are arguing about not supporting hardware that exists in the real world for vibes reasons rather than it being a realistically attackable weakness (and if we really *are* that concerned about SHA-1, why are we still supporting TPM 1.2 at all?)
On Fri Aug 16, 2024 at 9:41 PM EEST, Matthew Garrett wrote: > On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote: > > > For (any) non-legacy features we can choose, which choices we choose to > > support, and which we do not. This is not an oppositive view just saying > > how it is, and platforms set of choices is not a selling argument. > > NIST still permits the use of SHA-1 until 2030, and the most significant > demonstrated weaknesses in it don't seem applicable to the use case > here. We certainly shouldn't encourage any new uses of it, and anyone > who's able to use SHA-2 should be doing that instead, but it feels like > people are arguing about not supporting hardware that exists in the real > world for vibes reasons rather than it being a realistically attackable > weakness (and if we really *are* that concerned about SHA-1, why are we > still supporting TPM 1.2 at all?) We are life-supporting TPM 1.2 as long as necessary but neither the support is extended nor new features will gain TPM 1.2 support. So that is at least my policy for that feature. BR, Jarkko
On Mon, Aug 19, 2024 at 09:05:47PM +0300, Jarkko Sakkinen wrote: > On Fri Aug 16, 2024 at 9:41 PM EEST, Matthew Garrett wrote: > > On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote: > > > > > For (any) non-legacy features we can choose, which choices we choose to > > > support, and which we do not. This is not an oppositive view just saying > > > how it is, and platforms set of choices is not a selling argument. > > > > NIST still permits the use of SHA-1 until 2030, and the most significant > > demonstrated weaknesses in it don't seem applicable to the use case > > here. We certainly shouldn't encourage any new uses of it, and anyone > > who's able to use SHA-2 should be doing that instead, but it feels like > > people are arguing about not supporting hardware that exists in the real > > world for vibes reasons rather than it being a realistically attackable > > weakness (and if we really *are* that concerned about SHA-1, why are we > > still supporting TPM 1.2 at all?) > > We are life-supporting TPM 1.2 as long as necessary but neither the > support is extended nor new features will gain TPM 1.2 support. So > that is at least my policy for that feature. But the fact that we support it and provide no warning labels is a pretty clear indication that we're not actively trying to prevent people from using SHA-1 in the general case. Why is this a different case? Failing to support it actually opens an entire separate set of footgun opportunities in terms of the SHA-1 banks now being out of sync with the SHA-2 ones, so either way we're leaving people open to making poor choices.
On Mon Aug 19, 2024 at 9:24 PM EEST, Matthew Garrett wrote: > On Mon, Aug 19, 2024 at 09:05:47PM +0300, Jarkko Sakkinen wrote: > > On Fri Aug 16, 2024 at 9:41 PM EEST, Matthew Garrett wrote: > > > On Fri, Aug 16, 2024 at 02:22:04PM +0300, Jarkko Sakkinen wrote: > > > > > > > For (any) non-legacy features we can choose, which choices we choose to > > > > support, and which we do not. This is not an oppositive view just saying > > > > how it is, and platforms set of choices is not a selling argument. > > > > > > NIST still permits the use of SHA-1 until 2030, and the most significant > > > demonstrated weaknesses in it don't seem applicable to the use case > > > here. We certainly shouldn't encourage any new uses of it, and anyone > > > who's able to use SHA-2 should be doing that instead, but it feels like > > > people are arguing about not supporting hardware that exists in the real > > > world for vibes reasons rather than it being a realistically attackable > > > weakness (and if we really *are* that concerned about SHA-1, why are we > > > still supporting TPM 1.2 at all?) > > > > We are life-supporting TPM 1.2 as long as necessary but neither the > > support is extended nor new features will gain TPM 1.2 support. So > > that is at least my policy for that feature. > > But the fact that we support it and provide no warning labels is a > pretty clear indication that we're not actively trying to prevent people > from using SHA-1 in the general case. Why is this a different case? > Failing to support it actually opens an entire separate set of footgun > opportunities in terms of the SHA-1 banks now being out of sync with the > SHA-2 ones, so either way we're leaving people open to making poor > choices. This is a fair and enclosing argument. I get where you are coming from now. Please as material for the commit message. BR, Jarkko
On 8/15/24 15:10, Thomas Gleixner wrote: > On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: >> On 5/31/24 09:54, Eric W. Biederman wrote: >>> Eric Biggers <ebiggers@kernel.org> writes: >>>> That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use >>>> SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is >>>> that the case? Sure, maybe there are situations where you *have* to use SHA-1, >>>> but why would you not at least *prefer* SHA-256? >>> >>> Yes. Please prefer to use SHA-256. >>> >>> Have you considered implementing I think it is SHA1-DC (as git has) that >>> is compatible with SHA1 but blocks the known class of attacks where >>> sha1 is actively broken at this point? >> >> We are using the kernel's implementation, addressing what the kernel >> provides is beyond our efforts. Perhaps someone who is interested in >> improving the kernel's SHA1 could submit a patch implementing/replacing >> it with SHA1-DC, as I am sure the maintainers would welcome the help. > > Well, someone who is interested to get his "secure" code merged should > have a vested interested to have a non-broken SHA1 implementation if > there is a sensible requirement to use SHA1 in that new "secure" code, > no? > > Just for the record. The related maintainers can rightfully decide to > reject known broken "secure" code on a purely technical argument. > > Thanks, > > tglx > There is one simple question, does allowing the Secure Launch code to record SHA1 measurements make the system insecure, and the answer is absolutely not. The role of the Secure Launch code base in the context of the larger launch process is to function as observer. Within this role, its only responsibility is continuing the trust chain(s) that were started by the CPU/Hardware. It does so by measuring the components and configuration it is responsible for loading and applying, i.e. in TCG parlance, it is continuing the construction of the transitive trust for the system. In this aspect, the only degradation of security that can affect the kernel's role is whether all the necessary entities are safely measured and not what algorithms are used. If the system integrator, whether that be the OEM, your employer, the distro maintainer, the system administrator, or the end user, configures the DL preamble to only use SHA1 or used older hardware that has a TPM1.2, then they are accepting the risk it creates in their solution. In fact, a greater threat to the security of the launch is the misconfiguration of the IOMMU, which risks the kernel's ability to safely make measurements, as compared to the use of SHA1. Yet it was insisted in past reviews that we allow the user to specify an incorrect IOMMU policy. In the end, the "security" of an RTM solution is how and what measurements are used to assess the health of a system. Thus bringing it back to the opening question, if SHA1 measurements are made but not used, i.e. the attestation enforcement only uses SHA2, then it has zero impact on the security of the system. Another fact to consider is that the current Intel's TXT MLE specification dictates SHA1 as a valid configuration. Secure Launch's use of SHA1 is therefore to comply with Intel's specification for TXT. And like the IOMMU situation, having the option available allows the user to determine how they ultimately want to integrate Secure Launch into their integrity management. And because Secure Launch will only attempt SHA1 if it was in the TXT configuration, when either Intel removes SHA1 from the MLE specification or firmware manufactures begin disabling the SHA1 banks, this will obviously mean that Secure Launch will not produce SHA1 measurements. On a side note, with my remote attestation hat on, the SHA1 measurements can in fact be extremely useful. If an attestation was made containing both SHA1 and SHA2 chains, and the SHA1 of an event was correct but the SHA2 was not, either a natural collision happened or someone maliciously caused a collision. The former has an extremely low probability, while the latter is highly probable. Thus, with this information alone, it is possible to make the reasonable determination the device is compromised. Whereas if both hashes are mismatched, without any additional information it is equally probable of either misconfiguration or compromise. And to state the obvious, with only SHA2, further information is needed to distinguish between misconfiguration and compromise. V/r, Daniel P. Smith
On Thu, May 30, 2024 at 07:16:56PM -0700, Eric Biggers wrote: > On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote: > > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > > > 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 <dpsmith@apertussolutions.com> > > Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > > Thanks. This explanation doesn't seem to have made it into the actual code or > documentation. Can you please get it into a more permanent location? I see that a new version of the patchset was sent out but this suggestion was not taken. Are you planning to address it? - Eric
On 8/27/24 11:14 AM, 'Eric Biggers' via trenchboot-devel wrote: > On Thu, May 30, 2024 at 07:16:56PM -0700, Eric Biggers wrote: >> On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> 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 <dpsmith@apertussolutions.com> >>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com> >> >> Thanks. This explanation doesn't seem to have made it into the actual code or >> documentation. Can you please get it into a more permanent location? > > I see that a new version of the patchset was sent out but this suggestion was > not taken. Are you planning to address it? Sorry we sort of overlooked that part of the request. We will take the latest commit message, clean it up a little and put it in boot/compressed/sha1.c file as a comment. I believe that is what you would like us to do. Thanks Ross > > - Eric >
On Wed, Aug 28, 2024 at 01:14:45PM -0700, ross.philipson@oracle.com wrote: > On 8/27/24 11:14 AM, 'Eric Biggers' via trenchboot-devel wrote: > > On Thu, May 30, 2024 at 07:16:56PM -0700, Eric Biggers wrote: > > > On Thu, May 30, 2024 at 06:03:18PM -0700, Ross Philipson wrote: > > > > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > > > > > > > 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 <dpsmith@apertussolutions.com> > > > > Signed-off-by: Ross Philipson <ross.philipson@oracle.com> > > > > > > Thanks. This explanation doesn't seem to have made it into the actual code or > > > documentation. Can you please get it into a more permanent location? > > > > I see that a new version of the patchset was sent out but this suggestion was > > not taken. Are you planning to address it? > > Sorry we sort of overlooked that part of the request. We will take the > latest commit message, clean it up a little and put it in > boot/compressed/sha1.c file as a comment. I believe that is what you would > like us to do. > Do users of this feature need to make a decision about SHA-1? If so there needs to be guidance in Documentation/. A comment in a .c file is not user facing. - Eric
On Thu, Aug 15, 2024 at 12:10 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: > > On 5/31/24 09:54, Eric W. Biederman wrote: > >> Eric Biggers <ebiggers@kernel.org> writes: > >>> That paragraph is also phrased as a hypothetical, "Even if we'd prefer to use > >>> SHA-256-only". That implies that you do not, in fact, prefer SHA-256 only. Is > >>> that the case? Sure, maybe there are situations where you *have* to use SHA-1, > >>> but why would you not at least *prefer* SHA-256? > >> > >> Yes. Please prefer to use SHA-256. > >> > >> Have you considered implementing I think it is SHA1-DC (as git has) that > >> is compatible with SHA1 but blocks the known class of attacks where > >> sha1 is actively broken at this point? > > > > We are using the kernel's implementation, addressing what the kernel > > provides is beyond our efforts. Perhaps someone who is interested in > > improving the kernel's SHA1 could submit a patch implementing/replacing > > it with SHA1-DC, as I am sure the maintainers would welcome the help. > > Well, someone who is interested to get his "secure" code merged should > have a vested interested to have a non-broken SHA1 implementation if > there is a sensible requirement to use SHA1 in that new "secure" code, > no? > > Just for the record. The related maintainers can rightfully decide to > reject known broken "secure" code on a purely technical argument. > Wait, hold on a second. SHA1-DC isn't SHA1. It's a different hash function that is mostly compatible with SHA1, is different on some inputs, and is maybe more secure. But the _whole point_ of using SHA1 in the TPM code (well, this really should be the whole point for new applications) is to correctly cap the SHA1 PCRs so we can correctly _turn them off_ in the best way without breaking compatibility with everything that might read the event log. I think that anyone suggesting using SHA1-DC for this purpose should give some actual analysis as to why they think it's an improvement, let alone even valid. Ross et al, can you confirm that your code actually, at least by default and with a monstrous warning to anyone who tries to change the default, caps SHA1 PCRs if SHA256 is available? And then can we maybe all stop hassling the people trying to develop this series about the fact that they're doing their best with the obnoxious system that the TPM designers gave them? Thanks, Andy
On Wed, Aug 28, 2024 at 08:17:05PM -0700, Andy Lutomirski wrote: > Ross et al, can you confirm that your code actually, at least by > default and with a monstrous warning to anyone who tries to change the > default, caps SHA1 PCRs if SHA256 is available? And then can we maybe > all stop hassling the people trying to develop this series about the > fact that they're doing their best with the obnoxious system that the > TPM designers gave them? Presumably this would be dependent upon non-SHA1 banks being enabled?
On Wed, Aug 28, 2024 at 8:25 PM Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > On Wed, Aug 28, 2024 at 08:17:05PM -0700, Andy Lutomirski wrote: > > > Ross et al, can you confirm that your code actually, at least by > > default and with a monstrous warning to anyone who tries to change the > > default, caps SHA1 PCRs if SHA256 is available? And then can we maybe > > all stop hassling the people trying to develop this series about the > > fact that they're doing their best with the obnoxious system that the > > TPM designers gave them? > > Presumably this would be dependent upon non-SHA1 banks being enabled? Of course. It's also not immediately obvious to me what layer of the stack should be responsible for capping SHA1 PCRs. Should it be the kernel? Userspace? It seems like a whole lot of people, for better or for worse, want to minimize the amount of code that even knows how to compute SHA1 hashes. I'm not personally convinced I agree with this strategy, but it is what it is. And maybe people would be happier if the default behavior of the kernel is to notice that SHA256 is available and then cap SHA1 before even asking user code's permission. --Andy
Hey again, On 9/4/24 21:01, Daniel P. Smith wrote: > Hi Luto. > > On 8/28/24 23:17, Andy Lutomirski wrote: >> On Thu, Aug 15, 2024 at 12:10 PM Thomas Gleixner <tglx@linutronix.de> >> wrote: >>> >>> On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: >>>> On 5/31/24 09:54, Eric W. Biederman wrote: >>>>> Eric Biggers <ebiggers@kernel.org> writes: >>>>>> That paragraph is also phrased as a hypothetical, "Even if we'd >>>>>> prefer to use >>>>>> SHA-256-only". That implies that you do not, in fact, prefer >>>>>> SHA-256 only. Is >>>>>> that the case? Sure, maybe there are situations where you *have* >>>>>> to use SHA-1, >>>>>> but why would you not at least *prefer* SHA-256? >>>>> >>>>> Yes. Please prefer to use SHA-256. >>>>> >>>>> Have you considered implementing I think it is SHA1-DC (as git has) >>>>> that >>>>> is compatible with SHA1 but blocks the known class of attacks where >>>>> sha1 is actively broken at this point? >>>> >>>> We are using the kernel's implementation, addressing what the kernel >>>> provides is beyond our efforts. Perhaps someone who is interested in >>>> improving the kernel's SHA1 could submit a patch implementing/replacing >>>> it with SHA1-DC, as I am sure the maintainers would welcome the help. >>> >>> Well, someone who is interested to get his "secure" code merged should >>> have a vested interested to have a non-broken SHA1 implementation if >>> there is a sensible requirement to use SHA1 in that new "secure" code, >>> no? >>> >>> Just for the record. The related maintainers can rightfully decide to >>> reject known broken "secure" code on a purely technical argument. >>> >> >> Wait, hold on a second. >> >> SHA1-DC isn't SHA1. It's a different hash function that is mostly >> compatible with SHA1, is different on some inputs, and is maybe more >> secure. But the _whole point_ of using SHA1 in the TPM code (well, >> this really should be the whole point for new applications) is to >> correctly cap the SHA1 PCRs so we can correctly _turn them off_ in the >> best way without breaking compatibility with everything that might >> read the event log. I think that anyone suggesting using SHA1-DC for >> this purpose should give some actual analysis as to why they think >> it's an improvement, let alone even valid. > > I would say at a minimum it is to provide a means to cap the PCRs. > Devices with TPM1.2 are still prevalent in the wild for which members of > the TrenchBoot community support, and there are still valid (and secure) > verification uses for SHA1 that I outlined in my previous response. > >> Ross et al, can you confirm that your code actually, at least by >> default and with a monstrous warning to anyone who tries to change the >> default, caps SHA1 PCRs if SHA256 is available? And then can we maybe >> all stop hassling the people trying to develop this series about the >> fact that they're doing their best with the obnoxious system that the >> TPM designers gave them? > > Our goal is to keep control in the hands of the user, not making > unilateral decisions on their behalf. In the currently deployed > solutions it is left to the initrd (user) to cap the PCRs. After some > thinking, we can still ensure user control and give an option to cap the > PCRs earlier. We hope to post a v11 later this week or early next week > that introduces a new policy field to the existing measurement policy > framework. Will add/update the kernel docs with respect to the policy > expansion. We are also looking the best way we might add a warning to > the kernel log if the SHA1 bank is used beyond capping the PCRs. As the attempt was made to lay in the policy logic, it started to become convoluted and unnecessarily complicated. Thus creating more risk with all the bookkeeping and yet sha1 hashes still have to be sent, the null hash in this case, since the TPM driver will reject extends that do not have hashes for all active banks. At this point, we have opted to keep the logic simple and add a section to our documentation advising of the potential risk should one choose to incorporate SHA1 in their attestations of the platform. v/r, dps
On Thu, Sep 12, 2024 at 5:34 PM Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > Hey again, > > On 9/4/24 21:01, Daniel P. Smith wrote: > > Hi Luto. > > > > On 8/28/24 23:17, Andy Lutomirski wrote: > >> On Thu, Aug 15, 2024 at 12:10 PM Thomas Gleixner <tglx@linutronix.de> > >> wrote: > >>> > >>> On Thu, Aug 15 2024 at 13:38, Daniel P. Smith wrote: > >>>> On 5/31/24 09:54, Eric W. Biederman wrote: > >>>>> Eric Biggers <ebiggers@kernel.org> writes: > >>>>>> That paragraph is also phrased as a hypothetical, "Even if we'd > >>>>>> prefer to use > >>>>>> SHA-256-only". That implies that you do not, in fact, prefer > >>>>>> SHA-256 only. Is > >>>>>> that the case? Sure, maybe there are situations where you *have* > >>>>>> to use SHA-1, > >>>>>> but why would you not at least *prefer* SHA-256? > >>>>> > >>>>> Yes. Please prefer to use SHA-256. > >>>>> > >>>>> Have you considered implementing I think it is SHA1-DC (as git has) > >>>>> that > >>>>> is compatible with SHA1 but blocks the known class of attacks where > >>>>> sha1 is actively broken at this point? > >>>> > >>>> We are using the kernel's implementation, addressing what the kernel > >>>> provides is beyond our efforts. Perhaps someone who is interested in > >>>> improving the kernel's SHA1 could submit a patch implementing/replacing > >>>> it with SHA1-DC, as I am sure the maintainers would welcome the help. > >>> > >>> Well, someone who is interested to get his "secure" code merged should > >>> have a vested interested to have a non-broken SHA1 implementation if > >>> there is a sensible requirement to use SHA1 in that new "secure" code, > >>> no? > >>> > >>> Just for the record. The related maintainers can rightfully decide to > >>> reject known broken "secure" code on a purely technical argument. > >>> > >> > >> Wait, hold on a second. > >> > >> SHA1-DC isn't SHA1. It's a different hash function that is mostly > >> compatible with SHA1, is different on some inputs, and is maybe more > >> secure. But the _whole point_ of using SHA1 in the TPM code (well, > >> this really should be the whole point for new applications) is to > >> correctly cap the SHA1 PCRs so we can correctly _turn them off_ in the > >> best way without breaking compatibility with everything that might > >> read the event log. I think that anyone suggesting using SHA1-DC for > >> this purpose should give some actual analysis as to why they think > >> it's an improvement, let alone even valid. > > > > I would say at a minimum it is to provide a means to cap the PCRs. > > Devices with TPM1.2 are still prevalent in the wild for which members of > > the TrenchBoot community support, and there are still valid (and secure) > > verification uses for SHA1 that I outlined in my previous response. > > > >> Ross et al, can you confirm that your code actually, at least by > >> default and with a monstrous warning to anyone who tries to change the > >> default, caps SHA1 PCRs if SHA256 is available? And then can we maybe > >> all stop hassling the people trying to develop this series about the > >> fact that they're doing their best with the obnoxious system that the > >> TPM designers gave them? > > > > Our goal is to keep control in the hands of the user, not making > > unilateral decisions on their behalf. In the currently deployed > > solutions it is left to the initrd (user) to cap the PCRs. After some > > thinking, we can still ensure user control and give an option to cap the > > PCRs earlier. We hope to post a v11 later this week or early next week > > that introduces a new policy field to the existing measurement policy > > framework. Will add/update the kernel docs with respect to the policy > > expansion. We are also looking the best way we might add a warning to > > the kernel log if the SHA1 bank is used beyond capping the PCRs. > > As the attempt was made to lay in the policy logic, it started to become > convoluted and unnecessarily complicated. Thus creating more risk with > all the bookkeeping and yet sha1 hashes still have to be sent, the null > hash in this case, since the TPM driver will reject extends that do not > have hashes for all active banks. At this point, we have opted to keep > the logic simple and add a section to our documentation advising of the > potential risk should one choose to incorporate SHA1 in their > attestations of the platform. > I've read the TPM standard a bit, but it's been awhile, and it's too complicated anyway. So, can you remind me (and probably 3/4 of the other people on this thread, too): What, exactly, is your patchset doing that requires hashing at all? (I assume it's extending a PCR and generating an event log entry.). What, exactly, does it mean to "cap" a PCR? How is this different from what your patchset does? With that answered, it will hopefully be easy to see that you're making the right call :) --Andy
On Sat, Sep 21, 2024 at 11:37 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > On 9/13/24 23:57, Andy Lutomirski wrote: > > On Thu, Sep 12, 2024 at 5:34 PM Daniel P. Smith > > <dpsmith@apertussolutions.com> wrote: > >> > > What, exactly, is your patchset doing that requires hashing at all? > > (I assume it's extending a PCR and generating an event log entry.). > > What, exactly, does it mean to "cap" a PCR? How is this different > > from what your patchset does? > > ... > I did not see the term actually defined in the client profile, but the > term "cap" refers to the specific action of hashing a value across a set > of PCRs. This is to reflect that certain events have occurred and will > result in a different but predictable change to the PCR value. Often > times this is to ensure that if there are TPM objects sealed to the > system with either that event having or have not occurred, they cannot > be unsealed. Thus, one has "capped" the PCRs as a means to close access > to the “acceptable” system state. Okay, so I read Ross's earlier email rather differently: > 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. I assumed that "deliberately cap" meant that there was an actual feature where you write something to the event log (if applicable) and extend the PCR in a special way that *turns that PCR off*. That is, it does something such that later-loaded software *can't* use that PCR to attest or unseal anything, etc. But it sounds like you're saying that no such feature exists. And a quick skim of the specs doesn't come up with anything. And the SHA1 banks may well be susceptible to a collision attack. So what are the kernel's choices wrt the SHA-1 PCRs? It can: a) Perform business as usual: extend them consistently with the SHA-256 PCRs. This is sort of *fine*: the kernel code in question is not relying on the security of SHA-1, but it is making it possible for future code to (unwisely) rely on them. (Although, if the kernel is loading a trustworthy initramfs, then there won't be a collision, and there is no known second-preimage attack against SHA-1.) b) Same as (a), but with countermeasures: do something to the effect of *detecting* the attack a la SHA1-DC and panic if an attack is detected. Maybe this is wise; maybe it's not. c) Do not extend the SHA-1 PCRs and pretend they don't exist. This seems likely to cause massive security problems, and having the kernel try to defend its behavior by saying "we don't support SHA-1 -- this is a problem downstream" seems unwise to me. d) Extend them but in an unconventional way that makes using them extra secure. For example, calculate SHA-256(next stage), then extend with (next stage || "Linux thinks this is better" || SHA-256(next stage). This makes the SHA-1 banks usable, and it seems like it will probably defeat anything resembling a current attack. But maybe this is silly. It would probably require doing the same thing to the SHA-256 banks for the benefit of any software that checks whether the SHA-1 and SHA-256 banks are consistent with each other. e) Actually try to make the SHA-1 PCRs unusable. For example, extend them with random numbers. My inclination is that having some kind of Linux "policy" that SHA-1 is forbidden adds no actual security value. Option (a) honestly seems fine. Nothing in the kernel *relies* on the SHA-1 hash being secure. But option (b) also seems okay if someone is willing to put the effort into implementing it and creating a proper test case. But the description of all this could certainly do a better job of explaining what's going on. --Andy > [1] A future expansion of Secure Launch will be to enable usage of > Intel's Hardware Shield, link below, to provide runtime trustworthy > determination of SMM. The full extent of this capability can only be > achieved under a DRTM launch of the system with Intel TXT. When enabled, > this can be used to verify the SMM protections are in place and inform > the kernel's memory management which regions of memory are safe from SMM > tampering. > > https://www.intel.com/content/dam/www/central-libraries/us/en/documents/drtm-based-computing-whitepaper.pdf Wow. I skimmed this paper. What an overcomplicated solution to a problem that doesn't deserve to exist in the first place.
Hi Luto, My apologies, I missed this response and the active on v11 cause me to get an inquiry why I hadn't responded. On 9/21/24 18:40, Andy Lutomirski wrote: > On Sat, Sep 21, 2024 at 11:37 AM Daniel P. Smith > <dpsmith@apertussolutions.com> wrote: >> >> On 9/13/24 23:57, Andy Lutomirski wrote: >>> On Thu, Sep 12, 2024 at 5:34 PM Daniel P. Smith >>> <dpsmith@apertussolutions.com> wrote: >>>> > >>> What, exactly, is your patchset doing that requires hashing at all? >>> (I assume it's extending a PCR and generating an event log entry.). >>> What, exactly, does it mean to "cap" a PCR? How is this different >>> from what your patchset does? >> >> > > ... > >> I did not see the term actually defined in the client profile, but the >> term "cap" refers to the specific action of hashing a value across a set >> of PCRs. This is to reflect that certain events have occurred and will >> result in a different but predictable change to the PCR value. Often >> times this is to ensure that if there are TPM objects sealed to the >> system with either that event having or have not occurred, they cannot >> be unsealed. Thus, one has "capped" the PCRs as a means to close access >> to the “acceptable” system state. > > Okay, so I read Ross's earlier email rather differently: > >> 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. > > I assumed that "deliberately cap" meant that there was an actual > feature where you write something to the event log (if applicable) and > extend the PCR in a special way that *turns that PCR off*. That is, > it does something such that later-loaded software *can't* use that PCR > to attest or unseal anything, etc. > > But it sounds like you're saying that no such feature exists. And a > quick skim of the specs doesn't come up with anything. And the SHA1 > banks may well be susceptible to a collision attack. Correct, the only entity that can disable PCR banks is the firmware. When it initializes the TPM, it can disable banks/algorithms. After that, when an extend operation is done, the TPM is expecting an entry for all active PCR banks and the TPM itself does the extend hash that is stored into the PCRs. > So what are the kernel's choices wrt the SHA-1 PCRs? It can: > > a) Perform business as usual: extend them consistently with the > SHA-256 PCRs. This is sort of *fine*: the kernel code in question is > not relying on the security of SHA-1, but it is making it possible for > future code to (unwisely) rely on them. (Although, if the kernel is > loading a trustworthy initramfs, then there won't be a collision, and > there is no known second-preimage attack against SHA-1.) > > b) Same as (a), but with countermeasures: do something to the effect > of *detecting* the attack a la SHA1-DC and panic if an attack is > detected. Maybe this is wise; maybe it's not. > > c) Do not extend the SHA-1 PCRs and pretend they don't exist. This > seems likely to cause massive security problems, and having the kernel > try to defend its behavior by saying "we don't support SHA-1 -- this > is a problem downstream" seems unwise to me. I will chime in here to say that you can't ignore them, but you can send a fixed value, either well-known or junk, as the SHA1 value when doing the extend operation as you suggest in (e). > d) Extend them but in an unconventional way that makes using them > extra secure. For example, calculate SHA-256(next stage), then extend > with (next stage || "Linux thinks this is better" || SHA-256(next > stage). This makes the SHA-1 banks usable, and it seems like it will > probably defeat anything resembling a current attack. But maybe this > is silly. It would probably require doing the same thing to the > SHA-256 banks for the benefit of any software that checks whether the > SHA-1 and SHA-256 banks are consistent with each other. > > e) Actually try to make the SHA-1 PCRs unusable. For example, extend > them with random numbers. > > My inclination is that having some kind of Linux "policy" that SHA-1 > is forbidden adds no actual security value. Option (a) honestly seems > fine. Nothing in the kernel *relies* on the SHA-1 hash being secure. > But option (b) also seems okay if someone is willing to put the effort > into implementing it and creating a proper test case. Obviously, for the most part, we are in agreement. The one caveat is that I don't think the effort to shore-up SHA1 provides a good return on the costs it would incur. With no intent to disparage any one person, there generally will be two groups that would use SHA1. The first would be those limited by their platform and understand the risks. The second would be those attempting to do a cryptographic-based security solution that has either been living under a rock the last few years or has done zero research into the capabilities they are using for their solution. IMHO it is better to not inhibit the first group trying to save the latter group as the latter are always doomed to failure. > But the description of all this could certainly do a better job of > explaining what's going on. I would be glad to do so, and have tried several ways to explain it. Even working with multiple people that understand the problem to draft a better explanation. It would be greatly appreciated if you could provide what points you think should be clarified to better help convey the situation. > --Andy > >> [1] A future expansion of Secure Launch will be to enable usage of >> Intel's Hardware Shield, link below, to provide runtime trustworthy >> determination of SMM. The full extent of this capability can only be >> achieved under a DRTM launch of the system with Intel TXT. When enabled, >> this can be used to verify the SMM protections are in place and inform >> the kernel's memory management which regions of memory are safe from SMM >> tampering. >> >> https://www.intel.com/content/dam/www/central-libraries/us/en/documents/drtm-based-computing-whitepaper.pdf > > Wow. I skimmed this paper. What an overcomplicated solution to a > problem that doesn't deserve to exist in the first place. While we could have a long discussion over the merits of SMM, the fact we have to face is that it is here, and it is not going anywhere any time soon. I honestly found AMD's SMM Containerization (Appendix D of the AMD64 Architecture Programmer’s Manual - Volume 2) the better approach, and it saddens me that it is completely disabled. v/r, dps
On Sat, 2024-11-02 at 10:53 -0400, Daniel P. Smith wrote: > Hi Luto, > > My apologies, I missed this response and the active on v11 cause me > to > get an inquiry why I hadn't responded. > > On 9/21/24 18:40, Andy Lutomirski wrote: [...] > > I assumed that "deliberately cap" meant that there was an actual > > feature where you write something to the event log (if applicable) > > and extend the PCR in a special way that *turns that PCR off*. > > That is, it does something such that later-loaded software *can't* > > use that PCR to attest or unseal anything, etc. > > > > But it sounds like you're saying that no such feature exists. And > > a quick skim of the specs doesn't come up with anything. And the > > SHA1 banks may well be susceptible to a collision attack. > > Correct, the only entity that can disable PCR banks is the firmware. No, that's not correct. Any user can use TPM_PCR_Allocate to activate or deactivate individual banks. The caveat is the change is not implemented until the next TPM reset (which should involve a reboot). BIOS also gets to the TPM before the kernel does, so it can, in theory, check what banks a TPM has and call TPM_PCR_Allocate to change them. In practice, because this requires a reboot, this is usually only done from the BIOS menus not on a direct boot ... so you can be reasonably sure that whatever changes were made will stick. > When it initializes the TPM, it can disable banks/algorithms. After > that, when an extend operation is done, the TPM is expecting an entry > for all active PCR banks and the TPM itself does the extend hash that > is stored into the PCRs. This, also, is not quite correct: an extend is allowed to specify banks that don't exist (in which case nothing happens and no error is reported) and miss banks that do (in which case no extend is done to that bank). In the early days of TPM2, some BIOS implementations only extended sha1 for instance, meaning the sha256 banks were all zero when the kernel started. Even today, if you activate a bank the BIOS doesn't know about, it likely won't extend it. You can see this in VM boots with OVMF and software TPMs having esoteric banks like SM3. Regards, James
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 000000000000..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 <linux/init.h> +#include <linux/linkage.h> +#include <linux/string.h> +#include <asm/boot.h> +#include <asm/unaligned.h> + +#include "../../../../lib/crypto/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) +{ + u32 ws[SHA1_WORKSPACE_WORDS]; + + sha1_transform(digest, data, ws); + + memzero_explicit(ws, sizeof(ws)); +} + +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)) { + 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; + } + + if (len) + memcpy(sctx->buffer + partial, data, len); +} + +static void sha1_final(struct sha1_state *sctx, u8 *out) +{ + const int bit_offset = SHA1_BLOCK_SIZE - sizeof(__be64); + unsigned int partial = sctx->count % SHA1_BLOCK_SIZE; + __be64 *bits = (__be64 *)(sctx->buffer + bit_offset); + __be32 *digest = (__be32 *)out; + int i; + + sctx->buffer[partial++] = 0x80; + if (partial > bit_offset) { + memset(sctx->buffer + partial, 0x0, SHA1_BLOCK_SIZE - partial); + partial = 0; + + __sha1_transform(sctx->state, sctx->buffer); + } + + memset(sctx->buffer + partial, 0x0, bit_offset - partial); + *bits = cpu_to_be64(sctx->count << 3); + __sha1_transform(sctx->state, sctx->buffer); + + for (i = 0; i < SHA1_DIGEST_SIZE / sizeof(__be32); i++) + put_unaligned_be32(sctx->state[i], digest++); + + *sctx = (struct sha1_state){}; +} + +void sha1(const u8 *data, unsigned int len, u8 *out) +{ + struct sha1_state sctx = {0}; + + sha1_init(sctx.state); + sctx.count = 0; + sha1_update(&sctx, data, len); + sha1_final(&sctx, out); +} +EXPORT_SYMBOL(sha1); + MODULE_LICENSE("GPL");