Re: [PATCH v2] crypto: asymmetric_keys - bound PE section ranges in pefile_parse_binary

On Fri, Aug 14, 2026 at 11:06 PM Fabrice Derepas
<[email protected]> wrote:
>
> pefile_digest_pe_contents() hashes each PE section with
>
>         crypto_shash_update(desc, pebuf + ctx->secs[i].data_addr,
>                             ctx->secs[i].raw_data_size);
>
> where both data_addr and raw_data_size come straight from the untrusted PE
> section table. pefile_parse_binary() bounds the section table's location
> but not the sections' data ranges, and nothing else on the path checks
> them. A section with data_addr and/or raw_data_size pointing outside the
> image therefore reads past the pelen-byte buffer (CWE-125); a large
> raw_data_size makes it read far past the end and fault.
>
> Commit f7dd32c5179d ("crypto: asymmetric_keys - fix OOB read in
> pefile_digest_pe_contents") recently fixed a sibling underflow in this same
> function (the hashed_bytes + certs_size trailer computation); the
> per-section read was left unchecked.
>
> The digest loop runs only after verify_pkcs7_signature() succeeds, so it
> requires a validly signed image -- but not an attacker signing key. The
> PKCS#7 signature covers the SpcIndirectDataContent (a self-contained digest
> in the certificate table), not the live section bytes, so tampering
> data_addr in a publicly available signed image leaves the signature valid
> at that step while the out-of-bounds read fires when the digest is
> recomputed. It is thus reachable by a local CAP_SYS_BOOT user via
> kexec_file_load() with a tampered signed image; the access is an
> out-of-bounds read only, but a large raw_data_size faults, which is a
> denial of service when panic_on_oops is set.
>
> Validate each section's data range against the image when the section
> table is parsed, reusing the existing chkaddr() bounds macro. A malformed
> range is rejected with -ELIBBAD at parse time -- before signature
> verification and before any hashing -- so a bad image is never digested.
>
> Fixes: af316fc442ef ("pefile: Digest the PE binary and compare to the PKCS#7 data")
> Assisted-by: copilot-cli:claude-opus-4-6 frama-c
> Signed-off-by: Fabrice Derepas <[email protected]>
> ---
> v2:
>   - move the check into pefile_parse_binary() and reuse chkaddr(), per
>     Ignat Korchagin's review [1], so a malformed image is rejected at parse
>     time -- before signature verification and before hashing. v1 put the
>     check in the pefile_digest_pe_contents() section loop.
>   - because chkaddr() runs on every section, v2 also validates zero-size
>     (raw_data_size == 0) sections, which v1 and the shipping digest loop
>     skip. This only rejects a zero-size section whose data_addr points past
>     the image -- malformed input that real PEs never produce (.bss uses
>     PointerToRawData = 0, which passes) -- and matches the "don't start on
>     malformed data" intent.
>
> [1] https://lore.kernel.org/all/CAOs+rJXZ++7_o4L6LuT567FjdYsa=6NEki3qcFSRX+ehJxpoVg@mail.gmail.com/
>
> Tested under KASAN (CONFIG_KASAN_GENERIC, x86-64) with a KUnit case that
> calls verify_pefile_signature() on real images: a signed-then-tampered

Same as your other patch - any reason not to include this KUnit case?

> image (a section's data_addr set to pelen) is now rejected with -ELIBBAD in
> pefile_parse_binary(), before verify_pkcs7_signature() and with no KASAN
> splat, while an untampered signed image passes parse and reaches the
> signature check (-ENOKEY, as the test key is not trusted in this build).
> v1's KASAN repro -- a crafted context taken straight into the digest loop --
> showed the slab-out-of-bounds this prevents.
>
>  crypto/asymmetric_keys/verify_pefile.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
> index cec99db14..c447597f7 100644
> --- a/crypto/asymmetric_keys/verify_pefile.c
> +++ b/crypto/asymmetric_keys/verify_pefile.c
> @@ -30,6 +30,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
>         const struct data_dirent *dde;
>         const struct section_header *sec;
>         size_t cursor, datalen = pelen;
> +       unsigned int loop;
>
>         kenter("");
>
> @@ -112,6 +113,14 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
>                 return -ELIBBAD;
>         ctx->secs = pebuf + cursor;
>
> +       /* pefile_digest_pe_contents() hashes each section's raw data using
> +        * these fields directly; reject a section whose data range falls
> +        * outside the image so hashing never reads past the buffer.
> +        */
> +       for (loop = 0; loop < ctx->n_sections; loop++)

Looking at the code again: seems ctx->n_sections is user-controlled.
What happens if it is 0?

> +               chkaddr(0, ctx->secs[loop].data_addr,
> +                       ctx->secs[loop].raw_data_size);
> +
>         return 0;
>  }
>
>
> base-commit: d58772d8520c7ef247c4b95c9bd76d3a25da9ff5
> --
> 2.53.0
>
>

Ignat