Issue #522 has been updated by Werner Zeh.
Valerii Huhnin wrote in #note-8:
Werner Zeh wrote in #note-3:
Thanks for reporting this issue Vadim.
I wonder if we could instead avoid an overflow in the region_end() function. I have played around with this:
static inline size_t region_end(const struct region *r) { if (((size_t)MAX_SIZE - region_sz(r)) < region_offset(r)) return (size_t)MAX_SIZE; else return region_offset(r) + region_sz(r); }I guess this should be valid since a region which overflows the integer range isn't really meaningful. And according to my test it solves the case you have provided above. What do you think?
Werner
Dear Werner Zeh,
We considered implementing `region_end()` function like this. Implementing it this way is better than having no overflow checks at all, but it still does not work right on some edge cases. In particular, lets consider a region:
struct region r1 = {SIZE_MAX, 1};This is a region of size 1 which contains only one address, `SIZE_MAX` . But the proposed `region_end()` function would return `SIZE_MAX` as an end, instead of `SIZE_MAX + 1` (the latter of course can not be represented as a fixed-size integer). In this way it is effectively truncating a region with size 1 into a region with size 0. For example, here `sz1` would be 0 instead of 1:
size_t sz1 = region_end(&r1) - region_offset(&r1);Then, a region with size 0 of course can not overlap with anything (as it does not have any addresses in it), while a region with size 1 can overlap with other regions. For example, consider another region:
struct region r2 = {SIZE_MAX - 1, 2};This region contains two addresses: `SIZE_MAX - 1` and `SIZE_MAX`. It overlaps with `r1`, but the `region_overlaps()` function based on the proposed `region_end()` function would not detect this.
Valerii
Thanks for the explanation Valerii.
This is a region of size 1 which contains only one address, `SIZE_MAX` . But the proposed `region_end()` function would return `SIZE_MAX` as an end, instead of `SIZE_MAX + 1` (the latter of course can not be represented as a fixed-size integer). In this way it is effectively truncating a region with size 1 into a region with size 0.
I see...the issue is that region_end() actually computes the next address after the real end of the region, as Tim pointed out in his comment. I guess this is nothing we really want to have and which leads to the issues for corner cases. It seems to come from the way the comparison is done in region_overlap().
We could do the following, but I still need to check all use cases of region_end() as it now returns a different end address of the region:
``` c static inline size_t region_end(const struct region *r) { if (((size_t)MAX_SIZE - region_sz(r)) < region_offset(r)) return (size_t)MAX_SIZE; 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)); } ``` What do you think?
Werner
---------------------------------------- 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-1734
* 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)