[PATCH v1 0/2] Fix kexec of pesigned images

[PATCH v1 0/2] Fix kexec of pesigned images

[Robbie Harwood]

Hello,

In order to comply with the PE specification, recent versions of pesign do not
include 8-byte padding in the dwLength field.  kexec of signed images has
therefore not worked in Fedora (which uses pesign) for some time.

The first commit relaxes the check in order to fix this issue.  The second
upgrades several pe_debug() messages to pe_info() in order to make this more
debuggable on systems with secureboot lockdown in place.

Be well,
--Robbie

Robbie Harwood (2):
  verify_pefile: relax wrapper length check
  asymmetric_keys: log on fatal failures in PE/pkcs7

 crypto/asymmetric_keys/pkcs7_verify.c  | 10 ++++----
 crypto/asymmetric_keys/verify_pefile.c | 32 +++++++++++++++-----------
 2 files changed, 23 insertions(+), 19 deletions(-)

-- 
2.39.1

[PATCH v1 1/2] verify_pefile: relax wrapper length check

[Robbie Harwood]

The PE Format Specification (section "The Attribute Certificate Table
(Image Only)") states that `dwLength` is to be rounded up to 8-byte
alignment when used for traversal.  Therefore, the field is not required
to be an 8-byte multiple in the first place.

Accordingly, pesign has not performed this alignment since version
0.110.  This causes kexec failure on pesign'd binaries with "PEFILE:
Signature wrapper len wrong".  Update the comment and relax the check.

See-also: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-attribute-certificate-table-image-only
See-also: https://github.com/rhboot/pesign
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 crypto/asymmetric_keys/verify_pefile.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
index 7553ab18db89..fe1bb374239d 100644
--- a/crypto/asymmetric_keys/verify_pefile.c
+++ b/crypto/asymmetric_keys/verify_pefile.c
@@ -135,11 +135,15 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
 	pr_debug("sig wrapper = { %x, %x, %x }\n",
 		 wrapper.length, wrapper.revision, wrapper.cert_type);
 
-	/* Both pesign and sbsign round up the length of certificate table
-	 * (in optional header data directories) to 8 byte alignment.
+	/* sbsign rounds up the length of certificate table (in optional
+	 * header data directories) to 8 byte alignment.  However, the PE
+	 * specification states that while entries are 8-byte aligned, this is
+	 * not included in their length, and as a result, pesign has not
+	 * rounded up since 0.110.
 	 */
-	if (round_up(wrapper.length, 8) != ctx->sig_len) {
-		pr_debug("Signature wrapper len wrong\n");
+	if (wrapper.length > ctx->sig_len) {
+		pr_debug("Signature wrapper bigger than sig len (%x > %x)\n",
+			 ctx->sig_len, wrapper.length);
 		return -ELIBBAD;
 	}
 	if (wrapper.revision != WIN_CERT_REVISION_2_0) {
-- 
2.39.1

[PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7

[Robbie Harwood]

These particular errors can be encountered while trying to kexec when
secureboot lockdown is in place.  Without this change, even with a
signed debug build, one still needs to reboot the machine to add the
appropriate dyndbg parameters (since lockdown blocks debugfs).

Accordingly, upgrade all pr_debug() before fatal error into pr_info().

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c  | 10 +++++-----
 crypto/asymmetric_keys/verify_pefile.c | 24 ++++++++++++------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index f6321c785714..da425d142720 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -79,16 +79,16 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
 		}
 
 		if (sinfo->msgdigest_len != sig->digest_size) {
-			pr_debug("Sig %u: Invalid digest size (%u)\n",
-				 sinfo->index, sinfo->msgdigest_len);
+			pr_info("Sig %u: Invalid digest size (%u)\n",
+				sinfo->index, sinfo->msgdigest_len);
 			ret = -EBADMSG;
 			goto error;
 		}
 
 		if (memcmp(sig->digest, sinfo->msgdigest,
 			   sinfo->msgdigest_len) != 0) {
-			pr_debug("Sig %u: Message digest doesn't match\n",
-				 sinfo->index);
+			pr_info("Sig %u: Message digest doesn't match\n",
+				sinfo->index);
 			ret = -EKEYREJECTED;
 			goto error;
 		}
@@ -478,7 +478,7 @@ int pkcs7_supply_detached_data(struct pkcs7_message *pkcs7,
 			       const void *data, size_t datalen)
 {
 	if (pkcs7->data) {
-		pr_debug("Data already supplied\n");
+		pr_info("Data already supplied\n");
 		return -EINVAL;
 	}
 	pkcs7->data = data;
diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
index fe1bb374239d..c30e6610db26 100644
--- a/crypto/asymmetric_keys/verify_pefile.c
+++ b/crypto/asymmetric_keys/verify_pefile.c
@@ -74,7 +74,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
 		break;
 
 	default:
-		pr_debug("Unknown PEOPT magic = %04hx\n", pe32->magic);
+		pr_info("Unknown PEOPT magic = %04hx\n", pe32->magic);
 		return -ELIBBAD;
 	}
 
@@ -95,7 +95,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
 	ctx->certs_size = ddir->certs.size;
 
 	if (!ddir->certs.virtual_address || !ddir->certs.size) {
-		pr_debug("Unsigned PE binary\n");
+		pr_info("Unsigned PE binary\n");
 		return -ENODATA;
 	}
 
@@ -127,7 +127,7 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
 	unsigned len;
 
 	if (ctx->sig_len < sizeof(wrapper)) {
-		pr_debug("Signature wrapper too short\n");
+		pr_info("Signature wrapper too short\n");
 		return -ELIBBAD;
 	}
 
@@ -142,16 +142,16 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
 	 * rounded up since 0.110.
 	 */
 	if (wrapper.length > ctx->sig_len) {
-		pr_debug("Signature wrapper bigger than sig len (%x > %x)\n",
-			 ctx->sig_len, wrapper.length);
+		pr_info("Signature wrapper bigger than sig len (%x > %x)\n",
+			ctx->sig_len, wrapper.length);
 		return -ELIBBAD;
 	}
 	if (wrapper.revision != WIN_CERT_REVISION_2_0) {
-		pr_debug("Signature is not revision 2.0\n");
+		pr_info("Signature is not revision 2.0\n");
 		return -ENOTSUPP;
 	}
 	if (wrapper.cert_type != WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
-		pr_debug("Signature certificate type is not PKCS\n");
+		pr_info("Signature certificate type is not PKCS\n");
 		return -ENOTSUPP;
 	}
 
@@ -164,7 +164,7 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
 	ctx->sig_offset += sizeof(wrapper);
 	ctx->sig_len -= sizeof(wrapper);
 	if (ctx->sig_len < 4) {
-		pr_debug("Signature data missing\n");
+		pr_info("Signature data missing\n");
 		return -EKEYREJECTED;
 	}
 
@@ -198,7 +198,7 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
 		return 0;
 	}
 not_pkcs7:
-	pr_debug("Signature data not PKCS#7\n");
+	pr_info("Signature data not PKCS#7\n");
 	return -ELIBBAD;
 }
 
@@ -341,8 +341,8 @@ static int pefile_digest_pe(const void *pebuf, unsigned int pelen,
 	digest_size = crypto_shash_digestsize(tfm);
 
 	if (digest_size != ctx->digest_len) {
-		pr_debug("Digest size mismatch (%zx != %x)\n",
-			 digest_size, ctx->digest_len);
+		pr_info("Digest size mismatch (%zx != %x)\n",
+			digest_size, ctx->digest_len);
 		ret = -EBADMSG;
 		goto error_no_desc;
 	}
@@ -373,7 +373,7 @@ static int pefile_digest_pe(const void *pebuf, unsigned int pelen,
 	 * PKCS#7 certificate.
 	 */
 	if (memcmp(digest, ctx->digest, ctx->digest_len) != 0) {
-		pr_debug("Digest mismatch\n");
+		pr_info("Digest mismatch\n");
 		ret = -EKEYREJECTED;
 	} else {
 		pr_debug("The digests match!\n");
-- 
2.39.1

Re: [PATCH v1 1/2] verify_pefile: relax wrapper length check

[David Howells]

Robbie Harwood <rharwood@redhat.com> wrote:

> See-also: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-attribute-certificate-table-image-only
> See-also: https://github.com/rhboot/pesign

These should be "Link:" I think.

Apart from that:

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7

[David Howells]

Robbie Harwood <rharwood@redhat.com> wrote:

> These particular errors can be encountered while trying to kexec when
> secureboot lockdown is in place.  Without this change, even with a
> signed debug build, one still needs to reboot the machine to add the
> appropriate dyndbg parameters (since lockdown blocks debugfs).
> 
> Accordingly, upgrade all pr_debug() before fatal error into pr_info().

I wonder if they should then be pr_warn() instead.

> Signed-off-by: Robbie Harwood <rharwood@redhat.com>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH v1 1/2] verify_pefile: relax wrapper length check

[Jarkko Sakkinen]

On Fri, Feb 17, 2023 at 03:14:34PM -0500, Robbie Harwood wrote:
> The PE Format Specification (section "The Attribute Certificate Table
> (Image Only)") states that `dwLength` is to be rounded up to 8-byte
> alignment when used for traversal.  Therefore, the field is not required
> to be an 8-byte multiple in the first place.
> 
> Accordingly, pesign has not performed this alignment since version
> 0.110.  This causes kexec failure on pesign'd binaries with "PEFILE:
> Signature wrapper len wrong".  Update the comment and relax the check.
> 
> See-also: https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-attribute-certificate-table-image-only
> See-also: https://github.com/rhboot/pesign
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>

OK, makes sense but what does relaxing this bring up to the table?

I.e. I do get the argument but do not see the motivation.

> ---
>  crypto/asymmetric_keys/verify_pefile.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
> index 7553ab18db89..fe1bb374239d 100644
> --- a/crypto/asymmetric_keys/verify_pefile.c
> +++ b/crypto/asymmetric_keys/verify_pefile.c
> @@ -135,11 +135,15 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
>  	pr_debug("sig wrapper = { %x, %x, %x }\n",
>  		 wrapper.length, wrapper.revision, wrapper.cert_type);
>  
> -	/* Both pesign and sbsign round up the length of certificate table
> -	 * (in optional header data directories) to 8 byte alignment.
> +	/* sbsign rounds up the length of certificate table (in optional
> +	 * header data directories) to 8 byte alignment.  However, the PE
> +	 * specification states that while entries are 8-byte aligned, this is
> +	 * not included in their length, and as a result, pesign has not
> +	 * rounded up since 0.110.
>  	 */
> -	if (round_up(wrapper.length, 8) != ctx->sig_len) {
> -		pr_debug("Signature wrapper len wrong\n");
> +	if (wrapper.length > ctx->sig_len) {
> +		pr_debug("Signature wrapper bigger than sig len (%x > %x)\n",
> +			 ctx->sig_len, wrapper.length);
>  		return -ELIBBAD;
>  	}
>  	if (wrapper.revision != WIN_CERT_REVISION_2_0) {
> -- 
> 2.39.1
> 

BR, Jarkko

Re: [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7

[Jarkko Sakkinen]

On Fri, Feb 17, 2023 at 03:14:35PM -0500, Robbie Harwood wrote:
> These particular errors can be encountered while trying to kexec when
> secureboot lockdown is in place.  Without this change, even with a
> signed debug build, one still needs to reboot the machine to add the
> appropriate dyndbg parameters (since lockdown blocks debugfs).
> 
> Accordingly, upgrade all pr_debug() before fatal error into pr_info().
> 
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>

Eessentially this changes configuration to hard coded implementation.

No gain IMHO. If you are ready to patch the kernel you could live with
boot time dyndbg parameters.

> ---
>  crypto/asymmetric_keys/pkcs7_verify.c  | 10 +++++-----
>  crypto/asymmetric_keys/verify_pefile.c | 24 ++++++++++++------------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
> index f6321c785714..da425d142720 100644
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -79,16 +79,16 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
>  		}
>  
>  		if (sinfo->msgdigest_len != sig->digest_size) {
> -			pr_debug("Sig %u: Invalid digest size (%u)\n",
> -				 sinfo->index, sinfo->msgdigest_len);
> +			pr_info("Sig %u: Invalid digest size (%u)\n",
> +				sinfo->index, sinfo->msgdigest_len);
>  			ret = -EBADMSG;
>  			goto error;
>  		}
>  
>  		if (memcmp(sig->digest, sinfo->msgdigest,
>  			   sinfo->msgdigest_len) != 0) {
> -			pr_debug("Sig %u: Message digest doesn't match\n",
> -				 sinfo->index);
> +			pr_info("Sig %u: Message digest doesn't match\n",
> +				sinfo->index);
>  			ret = -EKEYREJECTED;
>  			goto error;
>  		}
> @@ -478,7 +478,7 @@ int pkcs7_supply_detached_data(struct pkcs7_message *pkcs7,
>  			       const void *data, size_t datalen)
>  {
>  	if (pkcs7->data) {
> -		pr_debug("Data already supplied\n");
> +		pr_info("Data already supplied\n");
>  		return -EINVAL;
>  	}
>  	pkcs7->data = data;
> diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
> index fe1bb374239d..c30e6610db26 100644
> --- a/crypto/asymmetric_keys/verify_pefile.c
> +++ b/crypto/asymmetric_keys/verify_pefile.c
> @@ -74,7 +74,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
>  		break;
>  
>  	default:
> -		pr_debug("Unknown PEOPT magic = %04hx\n", pe32->magic);
> +		pr_info("Unknown PEOPT magic = %04hx\n", pe32->magic);
>  		return -ELIBBAD;
>  	}
>  
> @@ -95,7 +95,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
>  	ctx->certs_size = ddir->certs.size;
>  
>  	if (!ddir->certs.virtual_address || !ddir->certs.size) {
> -		pr_debug("Unsigned PE binary\n");
> +		pr_info("Unsigned PE binary\n");
>  		return -ENODATA;
>  	}
>  
> @@ -127,7 +127,7 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
>  	unsigned len;
>  
>  	if (ctx->sig_len < sizeof(wrapper)) {
> -		pr_debug("Signature wrapper too short\n");
> +		pr_info("Signature wrapper too short\n");
>  		return -ELIBBAD;
>  	}
>  
> @@ -142,16 +142,16 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
>  	 * rounded up since 0.110.
>  	 */
>  	if (wrapper.length > ctx->sig_len) {
> -		pr_debug("Signature wrapper bigger than sig len (%x > %x)\n",
> -			 ctx->sig_len, wrapper.length);
> +		pr_info("Signature wrapper bigger than sig len (%x > %x)\n",
> +			ctx->sig_len, wrapper.length);
>  		return -ELIBBAD;
>  	}
>  	if (wrapper.revision != WIN_CERT_REVISION_2_0) {
> -		pr_debug("Signature is not revision 2.0\n");
> +		pr_info("Signature is not revision 2.0\n");
>  		return -ENOTSUPP;
>  	}
>  	if (wrapper.cert_type != WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> -		pr_debug("Signature certificate type is not PKCS\n");
> +		pr_info("Signature certificate type is not PKCS\n");
>  		return -ENOTSUPP;
>  	}
>  
> @@ -164,7 +164,7 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
>  	ctx->sig_offset += sizeof(wrapper);
>  	ctx->sig_len -= sizeof(wrapper);
>  	if (ctx->sig_len < 4) {
> -		pr_debug("Signature data missing\n");
> +		pr_info("Signature data missing\n");
>  		return -EKEYREJECTED;
>  	}
>  
> @@ -198,7 +198,7 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
>  		return 0;
>  	}
>  not_pkcs7:
> -	pr_debug("Signature data not PKCS#7\n");
> +	pr_info("Signature data not PKCS#7\n");
>  	return -ELIBBAD;
>  }
>  
> @@ -341,8 +341,8 @@ static int pefile_digest_pe(const void *pebuf, unsigned int pelen,
>  	digest_size = crypto_shash_digestsize(tfm);
>  
>  	if (digest_size != ctx->digest_len) {
> -		pr_debug("Digest size mismatch (%zx != %x)\n",
> -			 digest_size, ctx->digest_len);
> +		pr_info("Digest size mismatch (%zx != %x)\n",
> +			digest_size, ctx->digest_len);
>  		ret = -EBADMSG;
>  		goto error_no_desc;
>  	}
> @@ -373,7 +373,7 @@ static int pefile_digest_pe(const void *pebuf, unsigned int pelen,
>  	 * PKCS#7 certificate.
>  	 */
>  	if (memcmp(digest, ctx->digest, ctx->digest_len) != 0) {
> -		pr_debug("Digest mismatch\n");
> +		pr_info("Digest mismatch\n");
>  		ret = -EKEYREJECTED;
>  	} else {
>  		pr_debug("The digests match!\n");
> -- 
> 2.39.1
> 

BR, Jarkko