[L] Change in coreboot[main]: drivers/efi/uefi_capsules.c: coalesce and store UEFI capsules

[Nico Huber (Code Review)]

Attention is currently required from: Sergii Dmytruk.

Nico Huber has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/coreboot/+/83422?usp=email )

Change subject: drivers/efi/uefi_capsules.c: coalesce and store UEFI capsules
......................................................................

Patch Set 6:

(4 comments)

Patchset:

PS6: 
I admire the diligence that obviously went into this implementation,
it's nice, readable code. It's still C, though, and the issues I've
found show how dangerous that is. The overflows would be caught by
a safer language, the alignment issue seems easy to discover with
formal verification (e.g. by making the length of space written to
a post condition of the function).

Please note that programming mistakes in this code are only one
possible attack vector, as coreboot is unprepared for dealing with
untrusted input (same probably for early parts of a payload). What
happens for instance if the coalesced capsules fill 95%, 99%, 100%
of the RAM below 4GiB? This can have subtle effects on all the code
that runs from the coalescing to the actual verification inside the
payload, potentially making *any* hidden bug (also on program paths
that normally wouldn't be taken) exploitable. IMO a design flaw of
the SG approach that cannot be avoided without doing everything
right away in coreboot/PEI, making a secure implementation about
a hundred times more expensive than the alternatives.

File src/drivers/efi/capsules.c:

https://review.coreboot.org/c/coreboot/+/83422/comment/4d768f79_98f1cf90?usp... :
PS6, Line 347: 		data_size += ALIGN_UP(capsule_hdr->CapsuleImageSize, CAPSULE_ALIGNMENT);
This can overflow with a maliciously crafted SG list (e.g. one that contains
billions of references to the same, huge capsule). Without exploiting any
further issues, this would need 64GiB of SG blocks plus some more boilerplate,
making it feasible on systems with >=66GiB of RAM.

Overflowing here makes the coalescing code later write beyond the reserved space.

https://review.coreboot.org/c/coreboot/+/83422/comment/6f3021d5_49e783c6?usp... :
PS6, Line 387: 	*total_data_size += data_size;
Same here.

https://review.coreboot.org/c/coreboot/+/83422/comment/452dbe05_b9e88657?usp... :
PS6, Line 636: 				target += ALIGN_UP(block.len, CAPSULE_ALIGNMENT) - block.len;
This looks odd as the alignment accounted for is one of `CapsuleImageSize` which
doesn't have to match the alignment of `block.len`. Not as severe as the overflow
issue, but if I'm not mistaken it allows an adversary to write 217 bytes beyond
the reserved space. For instance with `CapsuleImageSize` of 40B (no alignment gap
accounted for), one block of 39B and one of 1B resulting in a wrong 7B increase
of `target` (times `MAX_CAPSULES - 1` gaps).