Issue #522 has been updated by Vadim Zaliva.
Werner Zeh wrote in #note-10:
... What do you think?
What you are proposing is to change the semantics of the `region_end` function to return the last element of the region instead of one-after. Given that it is a static function, the impact of such a change should be manageable; however, it is still a change in semantics. Perhaps renaming it to something more explicit, such as `region_last`, would aid in clarity.
Secondly, this modification will not operate correctly with empty regions (size 0). Consider, for example, a region with an offset of 0 and size of 0. One could argue that legitimate regions should not even have a size of zero, but this would need to be enforced elsewhere.
The enforcement argument is also pertinent to your suggestion of using a constructor function to ensure constraints on allowable values or region structure. Since we're dealing with just a C structure after all, I think it's insecure to rely solely on that. If, due to some other bug or exploit, someone managed to create a structure with unexpected values, `region_overlap` should be able to catch that.
---------------------------------------- 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-1735
* 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)