[PATCH] fix: check for integer underflow

[Michael Adler]

An integer underflow results in the attempt to SetMem a region of
impossible size and thus crashing the machine.
For example, this can happen if something is wrong with the image
assembly process (which was the case for me, even though I used
bg_gen_unified_kernel).
This commit doesn't change the fact that the UKI won't boot, but at
least it won't crash the machine and more importantly, it will tell you
exactly what's wrong and (hopefully) save you some debugging time.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
kernel-stub/main.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index c0be1f6..1e138c4 100644
--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -198,6 +198,13 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
kernel_image.ImageSize = kernel_section->VirtualSize;

CopyMem(kernel_image.ImageBase, kernel_source, kernel_image.ImageSize);
+
+ /* check for underflow, otherwise zeroing .bss crashes the machine */
+ if (pe_header->Opt.SizeOfImage < kernel_image.ImageSize) {
+ status = EFI_INVALID_PARAMETER;
+ error(L"Error: SizeOfImage smaller than kernel ImageSize", status);
+ goto cleanup_buffer;
+ }
/* Clear the rest so that .bss is definitely zero. */
SetMem((UINT8 *) kernel_image.ImageBase + kernel_image.ImageSize,
pe_header->Opt.SizeOfImage - kernel_image.ImageSize, 0);
--
2.38.1

[Jan Kiszka]

All that should be signed, so this is "just" a safety measure, right?

Is that enough, or should we look systematically for such things?

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

[Michael Adler]

Hi Jan,

> All that should be signed, so this is "just" a safety measure, right?

yes, this is just a convenience feature to give the user a proper error message instead of (hundreds of thousands)
synchronous exceptions.

> Is that enough, or should we look systematically for such things?

Well, I think this one was particularly "nasty" because it seems you will get a synchronous exception for *every*
invalid memory access (of which there are many due to the underflow). So I'd say it's enough for the time being.
However, I had to insert quite a few logging statements into the kernel stub to find out what's going on. My custom
kernel stub was quite verbose, which is probably not what you want by default, but I'd fancy a mechanism to turn on
verbose logging for the kernel stub (without having to recompile). I'm not too familiar with UEFI programming, so
I don't know how feasible that is.

Kind Regards,
Michael

--
Michael Adler

Siemens AG
T CED SES-DE
Otto-Hahn-Ring 6
81739 München, Deutschland

Siemens Aktiengesellschaft: Vorsitzender des Aufsichtsrats: Jim Hagemann Snabe; Vorstand: Roland Busch, Vorsitzender; Klaus Helmrich, Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese; Sitz der Gesellschaft: Berlin und München, Deutschland; Registergericht: Berlin-Charlottenburg, HRB 12300, München, HRB 6684; WEEE-Reg.-Nr. DE 23691322

[Jan Kiszka]

On 08.12.22 08:28, Michael Adler wrote:
> Hi Jan,
>
>> All that should be signed, so this is "just" a safety measure, right?
>
> yes, this is just a convenience feature to give the user a proper error message instead of (hundreds of thousands)
> synchronous exceptions.
>
>> Is that enough, or should we look systematically for such things?
>
> Well, I think this one was particularly "nasty" because it seems you will get a synchronous exception for *every*
> invalid memory access (of which there are many due to the underflow). So I'd say it's enough for the time being.
> However, I had to insert quite a few logging statements into the kernel stub to find out what's going on. My custom
> kernel stub was quite verbose, which is probably not what you want by default, but I'd fancy a mechanism to turn on
> verbose logging for the kernel stub (without having to recompile). I'm not too familiar with UEFI programming, so
> I don't know how feasible that is.
>

Rather than adding lots of runtime checks to the image, which is
specifically questionable in secure boot scenarios where the integrity
check comes first anyway, I wonder if we should rather improve the
generator script in this regard.

[Christian Storm]

Hi,

Both, if you ask me ;)

The generator script would ideally check this ― but you cannot rely on
every boot artifact being built by "our" script, and then the run-time
check(s) come in handy as last resort so to say.

I don't see how that touches integrity in Secure Boot scenarios? You can
boot perfectly integer artifacts that simply don't boot... It's more about
sematics here not syntax which is what integrity enforces.


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Jan Kiszka]

I don't mind adding runtime checks if they help in debugging not
completely unlikely issues.

But I'm even more interested in finding and avoiding issues between the
artifact production, UKI generation and finally signing possibly
corrupted things.

[Jan Kiszka]

I mean, checking for this obvious issue that there is an integer
overflow does not catch cases of a corrupted and too large SizeOfImage
field.

[Michael Adler]

> I don't mind adding runtime checks if they help in debugging not completely unlikely issues.

Since it happened to me and I was using bg_gen_unified_kernel, I'd say it's not completely unlikely :) Given that this
tool is rather new, I wouldn't be surprised if there are more bugs waiting be discovered in the future (it's just the
nature of software).

> But I'm even more interested in finding and avoiding issues between the artifact production, UKI generation and
> finally signing possibly corrupted things.

I agree, the actual problem is that bg_gen_unified_kernel generated an invalid UKI; it did produce a valid image though
once I enabled CONFIG_EFI in the kernel (which was missing for the corrupted image). I will try to reproduce this issue
after fixing another issue with U-Boot.

In any case, signing wouldn't have made a difference in my case: I would have just signed the EFI file with my developer
keys since I wanted to see if it actually boots (turns out, it didn't) :). This patch is not meant to protect from
malicious attackers, it's just supposed to make the life of developers more pleasant.

[Jan Kiszka]

On 08.12.22 15:02, Michael Adler wrote:
>> I don't mind adding runtime checks if they help in debugging not completely unlikely issues.
>
> Since it happened to me and I was using bg_gen_unified_kernel, I'd say it's not completely unlikely :) Given that this
> tool is rather new, I wouldn't be surprised if there are more bugs waiting be discovered in the future (it's just the
> nature of software).
>
>> But I'm even more interested in finding and avoiding issues between the artifact production, UKI generation and
>> finally signing possibly corrupted things.
>
> I agree, the actual problem is that bg_gen_unified_kernel generated an invalid UKI; it did produce a valid image though
> once I enabled CONFIG_EFI in the kernel (which was missing for the corrupted image). I will try to reproduce this issue
> after fixing another issue with U-Boot.

Ah, that is a good hint! Without that, the kernel carries no EFI stub,
thus has no PE header. That should be checked by the generator.