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

[Ignat Korchagin]

Sorry for delay on this,

On Sat, Aug 15, 2026 at 12:45 PM Fabrice Derepas
<fabrice.derepas@xxxxxxxxxxxxx> wrote:
>
> On Fri, Aug 14, 2026 at 11:29 PM Ignat Korchagin <ignat@xxxxxxxxx> wrote:
> > > 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?
>
> The reason I am not including my tests is that I use real signed EFI
> binaries (a valid one and a tampered one), ~88 KB each via xxd -i, which is
> a lot of binary to carry in-tree. For upstream I'll write a self-contained
> case that constructs a minimal PE in the test itself and asserts that
> pefile_parse_binary() rejects an out-of-range section, so there are no
> embedded blobs. I'll send it as a second patch in a v3 series, so the fix
> is not gated on the test.
>
> > > +       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?
>
> For this loop, nothing: n_sections is unsigned, so "loop < 0" is false on
> the first test and the body never runs -- no chkaddr() call and no
> ctx->secs[] dereference. So the patch itself adds no hazard for
> n_sections == 0.
>
> But you have pointed at a real pre-existing problem one step further on.
> n_sections == 0 is not rejected anywhere -- pefile_parse_binary() only
> bounds it from above, against header_size -- and pefile_digest_pe_contents()
> then does:
>
>         canon = kcalloc(ctx->n_sections, sizeof(unsigned), GFP_KERNEL);
>         if (!canon)
>                 return -ENOMEM;
>         ...
>         canon[0] = 0;
>
> kcalloc(0, ...) returns ZERO_SIZE_PTR, which is non-NULL, so the !canon
> check passes and the unconditional "canon[0] = 0" writes through
> ZERO_SIZE_PTR. Because the digest runs only after verify_pkcs7_signature()
> succeeds, this has the same reachability as the read this patch fixes: a
> validly signed image with n_sections tampered to 0 (the PKCS#7 is
> unaffected) reaches it via kexec_file_load().
>
> I confirmed it under KASAN. With a trusted key embedded, a signed image
> whose NumberOfSections I set to 0 (leaving the certificate table untouched,
> so verify_pkcs7_signature() still returns 0) faults in the digest step,
> while the untampered image verifies:
>
>   control (trusted, 7 sections): verify_pefile_signature() = 0
>   BUG: kernel NULL pointer dereference, address: 0000000000000010
>   #PF: supervisor write access in kernel mode
>   Oops: 0002 [#1] SMP KASAN NOPTI
>   RIP: 0010:verify_pefile_signature+0x5c3   (pefile_digest_pe_contents inlined)
>
> Address 0x10 is ZERO_SIZE_PTR and error_code 0x2 is a write -- the
> canon[0] = 0 store -- so it is a write fault, reachable the same way as the
> out-of-bounds read this patch already addresses.
>
> Since we are now centralising section-table sanity in pefile_parse_binary(),
> the natural fix is to reject n_sections == 0 there as well, e.g. by folding
> it into the existing bound:
>
>         if (ctx->n_sections == 0 ||
>             ctx->n_sections > (ctx->header_size - cursor) / sizeof(*sec))
>                 return -ELIBBAD;
>
> A PE with zero sections is malformed (a signed kernel/EFI image always has
> at least one), so rejecting it at parse is safe and closes the
> ZERO_SIZE_PTR write too. I will include that in v3 and cover it in the KUnit
> case.
>
> Do you prefer it folded into this patch, or kept as a separate fix for the

Let's fold into this patch, since it is one place now.

> canon[0] write? Either way I will respin as v3 with the KUnit case once you
> let me know.

Thank you.

>
> Thanks for the careful review, Ignat.
>
> Fabrice
>