Issue #522 has been updated by Maximilian Brune.
Tim Crawford wrote in #note-9:
Was the intent of `region_end()` really to return the address *after* the end of the region? By it's name, I would expect it to return `start + size - 1`, not where the next region would start.
I agree. Just returning the address of the last "element" would get rid of the edge case.
Werner Zeh wrote in #note-10:
static inline size_t region_end(const struct region *r) { if (((size_t)SIZE_MAX - region_sz(r)) < region_offset(r)) return (size_t)SIZE_MAX; else return region_offset(r) + region_sz(r) - 1; } static inline bool region_overlap(const struct region *r1, const struct region *r2) { return (region_end_new(r1) >= region_offset(r2)) && (region_offset(r1) <= region_end_new(r2)); }
I think it is easier/simpler to just let region_end return the address of the last element.
Regarding possible malicious input as mentioned in the issue: If we really have an untrusted source from which we get certain region values (e.g. SMM) it makes more sense to just always first check for sane input values and only call subsequent functions (like all region_* functions) if the values are sane. I don't think it makes sense to put any kind of special logic inside the region_* functions. Personally I think it is mostly confusing.
---------------------------------------- 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-1736
* 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)