Issue #522 has been updated by Valerii Huhnin.
I would like to formulate several questions, the answers to which correspond to trade-offs of various possible solutions of this issue.
* What are well-formed regions? * The last address of the region should be representable as a `size_t` value? * The last address of the region +1 should be representable as a `size_t` value? * Regions should not have size 0 ? * What functions can assume that regions are well-formed? * region API functions? * SMRAM overlap check functions? * But for sure not the SMI handlers themselves? * Where should we check if a region is well-formed? * Create a dedicated function to check well-formdness of regions? * Create a dedicated constructor function and check it there? * Check it directly in region API functions? * What should we do if we encounter a non-well-formed region? * Stop the execution? * Try to treat it as well-formed (e.g. by ignoring addresses outside of `SIZE_MAX`) ? * Other context-dependent handling?
Additionally, I would like to note that the definition of the well-formed values of the region struct should be based on what we are going to do with those values, and this goes beyond just the `region_overlap()` function. In particular, we are going to convert an address inside the region to a pointer and actually use this pointer to read and write data.
For example, in the `mainboard_smi_brigthness_down()` function we are constructing a pointer `*(bar + LVTMA_BL_MOD_LEVEL)`, and thus are performing the same addition that we perform during the evaluation of the `region_end()` function used in `region_overlap()` (well, almost the same, as we are constructing a pointer from the last address of the region, while the `region_end()` calculates the last address + 1).
Note: I also have a concern of the usage of the `size_t` type there. Shouldn't we actually use `uintptr_t`? Might there be problems when converting between this two?
After looking at the `mainboard_smi_brigthness_down()` function I think that regions whose last address is non-representable as `size_t` is a problem on its own (independently from whether or not `region_overlap()` correctly detects overlap), as having such regions would later lead to an attempt of constructing a pointer that points outside of the address space.
Thus, I think that ensuring that the last address of the region is representable as `size_t` is a reasonable requirement for the well-formed regions. We should not try to treat the regions that do not satisfy this requirement as well-formed as how we did in the our initial proposed implementation of the `region_overlap()` function.
Considering the above, now I propose: 1) To agree on the definition of well-formed regions; 2) 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; 3) Check and, if necessary, ensure that region API functions such as `region_overlap()` work correctly on all well-formed regions; 4) Call the region well-formdness check function directly inside the SMI handlers before the `smm_points_to_smram()` check function; 5) 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.
---------------------------------------- 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-1743
* 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)