diff mbox series

[v9,06/19] x86: Add early SHA-1 support for Secure Launch early measurements

Message ID 20240531010331.134441-7-ross.philipson@oracle.com
State Superseded
Headers show
Series x86: Trenchboot secure dynamic launch Linux kernel support | expand

Commit Message

Ross Philipson May 31, 2024, 1:03 a.m. UTC 
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

Comments

Eric Biggers May 31, 2024, 2:16 a.m. UTC | #1 
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?

> /*
>  * 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.

- Eric
Eric W. Biederman May 31, 2024, 1:54 p.m. UTC | #2 
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
Ross Philipson May 31, 2024, 4:18 p.m. UTC | #3 
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
Jarkko Sakkinen June 4, 2024, 6:52 p.m. UTC | #4 
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
Ross Philipson June 4, 2024, 9:02 p.m. UTC | #5 
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
Jarkko Sakkinen June 4, 2024, 10:40 p.m. UTC | #6 
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
Daniel P. Smith Aug. 15, 2024, 5:38 p.m. UTC | #7 
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
Thomas Gleixner Aug. 15, 2024, 7:10 p.m. UTC | #8 
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
Jarkko Sakkinen Aug. 16, 2024, 10:42 a.m. UTC | #9 
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
Andrew Cooper Aug. 16, 2024, 11:01 a.m. UTC | #10 
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
Jarkko Sakkinen Aug. 16, 2024, 11:22 a.m. UTC | #11 
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
Matthew Garrett Aug. 16, 2024, 6:41 p.m. UTC | #12 
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?)
Jarkko Sakkinen Aug. 19, 2024, 6:05 p.m. UTC | #13 
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
Matthew Garrett Aug. 19, 2024, 6:24 p.m. UTC | #14 
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.
Jarkko Sakkinen Aug. 20, 2024, 3:26 p.m. UTC | #15 
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
Daniel P. Smith Aug. 22, 2024, 6:29 p.m. UTC | #16 
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
Eric Biggers Aug. 27, 2024, 6:14 p.m. UTC | #17 
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
Ross Philipson Aug. 28, 2024, 8:14 p.m. UTC | #18 
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
>
Eric Biggers Aug. 28, 2024, 11:13 p.m. UTC | #19 
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
Andy Lutomirski Aug. 29, 2024, 3:17 a.m. UTC | #20 
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
Matthew Garrett Aug. 29, 2024, 3:25 a.m. UTC | #21 
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?
Andy Lutomirski Aug. 29, 2024, 5:26 p.m. UTC | #22 
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
Daniel P. Smith Sept. 5, 2024, 1:01 a.m. UTC | #23 
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.

Hopefully this answers the outstanding comments on the SHA1 thread.

v/r,
dps
Daniel P. Smith Sept. 13, 2024, 12:34 a.m. UTC | #24 
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
Andy Lutomirski Sept. 14, 2024, 3:57 a.m. UTC | #25 
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
Daniel P. Smith Sept. 21, 2024, 6:36 p.m. UTC | #26 
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:
>>
>> 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):

Sure, but honestly if you were to ask me in person, I would have given 
you the explanation as provided in the Secure Launch Overview in the 
documentation patch.

> 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?


The SINIT ACM is provided a structure that basically says, here is an 
address and size of what it will execute next. It will use that 
information to take its transitive trust measurement of the kernel 
before handing control to the Linux kernel. The Secure Launch code is 
responsible for ensuring everything that can influences its execution to 
be measured and stored into the TPM for attestations to be made at a 
latter time. The most important part is the transitive trust measurement 
of the next part to be executed, the initramfs. Specifically, the Secure 
Launch code must be able to handle the situation where the initramfs 
independent of the kernel and loaded separately. Additionally, the 
policy function provided for optional system state to also be measured 
and recorded, as the attestation evaluator might want them.

At the end of the day, this capability is strictly a passive (mostly, 
see note [1] below) solution with the responsibility to maintain the 
DRTM trust chain by taking meaningful measurements. This includes the 
next component in the trust chain and then hand execution to that next 
component.

The TCG specs and good practices provide that a component in either SRTM 
or DRTM trust chains should extend a non-event record to the tpm and/or 
its log. This is to indicate the transition point from one component in 
the trust chain to the next component. Under the client profile, 
firmware is required to do this by extending an event of type 
EV_SEPARATOR before "Ready to Boot".

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.

To close and reiterate, Secure Launch only responsibility is to send 
measurements to the TPM. The TPM and TPM driver has an expectation that 
every PCR extend event contains a hash for every active algorithm bank. 
For Secure Launch, to send SHA1 measurements has zero impact on the 
security of the system. Whether those measurements are used for TPM 
integrity reporting and security policy enforcement by user space or an 
enterprise is outside the scope of the Secure Launch capability and the 
kernel.

[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

> With that answered, it will hopefully be easy to see that you're
> making the right call :)
> 
> --Andy
>
Andy Lutomirski Sept. 21, 2024, 10:40 p.m. UTC | #27 
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.
Daniel P. Smith Nov. 2, 2024, 2:53 p.m. UTC | #28 
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
James Bottomley Nov. 2, 2024, 4:04 p.m. UTC | #29 
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 mbox series

Patch

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");