* [PATCH v1 0/2] Fix kexec of pesigned images
@ 2023-02-17 20:14 Robbie Harwood
2023-02-17 20:14 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check Robbie Harwood
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Robbie Harwood @ 2023-02-17 20:14 UTC (permalink / raw)
To: keyrings, David Howells; +Cc: 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v1 1/2] verify_pefile: relax wrapper length check 2023-02-17 20:14 [PATCH v1 0/2] Fix kexec of pesigned images Robbie Harwood @ 2023-02-17 20:14 ` Robbie Harwood 2023-02-17 21:11 ` Jarkko Sakkinen 2023-02-17 20:14 ` [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 Robbie Harwood ` (2 subsequent siblings) 3 siblings, 1 reply; 7+ messages in thread From: Robbie Harwood @ 2023-02-17 20:14 UTC (permalink / raw) To: keyrings, David Howells; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] verify_pefile: relax wrapper length check 2023-02-17 20:14 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check Robbie Harwood @ 2023-02-17 21:11 ` Jarkko Sakkinen 0 siblings, 0 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2023-02-17 21:11 UTC (permalink / raw) To: Robbie Harwood; +Cc: keyrings, David Howells 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 2023-02-17 20:14 [PATCH v1 0/2] Fix kexec of pesigned images Robbie Harwood 2023-02-17 20:14 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check Robbie Harwood @ 2023-02-17 20:14 ` Robbie Harwood 2023-02-17 21:14 ` Jarkko Sakkinen 2023-02-17 20:36 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check David Howells 2023-02-17 20:37 ` [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 David Howells 3 siblings, 1 reply; 7+ messages in thread From: Robbie Harwood @ 2023-02-17 20:14 UTC (permalink / raw) To: keyrings, David Howells; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 2023-02-17 20:14 ` [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 Robbie Harwood @ 2023-02-17 21:14 ` Jarkko Sakkinen 0 siblings, 0 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2023-02-17 21:14 UTC (permalink / raw) To: Robbie Harwood; +Cc: keyrings, David Howells 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] verify_pefile: relax wrapper length check 2023-02-17 20:14 [PATCH v1 0/2] Fix kexec of pesigned images Robbie Harwood 2023-02-17 20:14 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check Robbie Harwood 2023-02-17 20:14 ` [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 Robbie Harwood @ 2023-02-17 20:36 ` David Howells 2023-02-17 20:37 ` [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 David Howells 3 siblings, 0 replies; 7+ messages in thread From: David Howells @ 2023-02-17 20:36 UTC (permalink / raw) To: Robbie Harwood; +Cc: dhowells, keyrings 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 2023-02-17 20:14 [PATCH v1 0/2] Fix kexec of pesigned images Robbie Harwood ` (2 preceding siblings ...) 2023-02-17 20:36 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check David Howells @ 2023-02-17 20:37 ` David Howells 3 siblings, 0 replies; 7+ messages in thread From: David Howells @ 2023-02-17 20:37 UTC (permalink / raw) To: Robbie Harwood; +Cc: dhowells, keyrings 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-02-17 21:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-17 20:14 [PATCH v1 0/2] Fix kexec of pesigned images Robbie Harwood 2023-02-17 20:14 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check Robbie Harwood 2023-02-17 21:11 ` Jarkko Sakkinen 2023-02-17 20:14 ` [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 Robbie Harwood 2023-02-17 21:14 ` Jarkko Sakkinen 2023-02-17 20:36 ` [PATCH v1 1/2] verify_pefile: relax wrapper length check David Howells 2023-02-17 20:37 ` [PATCH v1 2/2] asymmetric_keys: log on fatal failures in PE/pkcs7 David Howells
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).