Issue #522 has been updated by Maximilian Brune.
- Implement a specialized function that checks if the region is well-formed and returns a boolean value. I prefer this approach over ensuring well-formdness of regions in the constructor function because this would allow us to separate the well-formdness check itself from the handling of non-well-formed regions, which would also allow us to handle non-well-formed regions differently in different contexts if needed;
I agree. The constructor should be separate from the check of itself. That gives us the ability to check for sane values where it makes sense (SMM) and leave them out for the rest (we can't check for overflows everywhere in the code base).
- Check and, if necessary, ensure that region API functions such as region_overlap() work correctly on all well-formed regions;
I agree. That check should assume that the sanity checks for the regions has already been done and just do a simple overlap check.
- Call the region well-formdness check function directly inside the SMI handlers before the smm_points_to_smram() check function;
Sounds good.
- If we encounter a non-well-fromed region in the SMI handler, just execute return similarly to how we do it when smm_points_to_smram() check is not passed. I prefer this approach over an assert as this would only stop the execution of the current function rather then the whole SMI handler and would allow the SMI handler to perform the required post-processing of the SMI.
I agree. Since the fault lies in whoever called the SMI handler, it should be their responsibility to check and debug the issues. The OS should be able to debug an SMI issue without always having to restart.
---------------------------------------- Bug #522: `region_overlap()` function might not work as expected due to an integer overflow in `region_end()` function. https://ticket.coreboot.org/issues/522#change-1744
* Author: Vadim Zaliva * Status: New * Priority: Normal * Category: coreboot common code * Target version: none * Start date: 2023-12-27 * Affected versions: master * Related links: https://review.coreboot.org/q/topic:enforce_region_api ---------------------------------------- `region_overlap()` function checks whether or not two memory regions overlap. Memory regions are represented as a region struct that contains the region's offset and size. This function then relies on `region_end()` function to compute the end of the region. `region_end()` function is susceptible to an integer overflow, which might result in the incorrect behaviour of `region_overlap()` function.
An example of inputs that lead to wrong behaviour: ``` struct region r1 = {SIZE_MAX - 10, 20}; struct region r2 = {SIZE_MAX - 20, 15}; ``` It returns 0, but since the regions actually overlap, it should return 1.
`region_overlap()` function is used in `smm_region_overlaps_handler()` function, which is itself used in SMI handlers to validate address values that come from an untrusted environment. This is necessary to prevent security vulnerabilities such as described in [BARing the System by Yuriy Bulygin, Oleksandr Bazhaniuk et al.](https://www.c7zero.info/stuff/REConBrussels2017_BARing_the_system.pdf)
We do not have an example of an exploit based on this incorrect behaviour and are aware of the existence of one. However, theoretically, this could lead to security vulnerabilities.
This bug was found during an ongoing [Coreboot Formal Verification Project](https://zaliva.org/UCSC-Twisted-Presentation-20231211.pdf), which aims to prove some important security properties of the coreboot’s SMI handler for the Gemini Lake/Octopus platform using Coq proof assistant and Verified Software Toolchain framework.
---Files-------------------------------- diff.txt (930 Bytes)