Issue #522 has been updated by Nico Huber.
Valerii Huhnin wrote in #note-14: > * What are well-formed regions? > * The last address of the region should be representable as a `size_t` > value? Definitely yes. > * The last address of the region +1 should be representable as a `size_t` > value? This won't be necessary and would only complicate the implementation. Everybody seems to agree that we don't want this. > * Regions should not have size 0 ? There seems to be consensus that we don't need it (and it would also minimally complicate the implementation). Though, we should do some testing because it is hard to assess if any part of the code expects this to work. > * What functions can assume that regions are well-formed? > * region API functions? > * SMRAM overlap check functions? > * But for sure not the SMI handlers themselves? We should check well-formedness before a `struct region` gets filled. Then any function using a struct region can assume that it's well formed. > * 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? We should explicitly check for invalid regions whenever data comes from outside of coreboot. We still need to decide on one of these two options. In any case, invalid external data should be handled gracefully. > * Check it directly in region API functions? We wouldn't want to do explicit error handling in every level of the API. IMO, an assert() in every public API function should suffice. > * 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? I'd say depending on the context as described above. > 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? They should be the same anyway. A `struct region` only sometimes describes a region in memory, so `uintptr_t` wouldn't always be right. > 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. This is true, but in this particular case, with the particular hardware, not a problem. The register should report 0 in the lower bits (except for the masked ones). This is also why the original overflow is hard to exploit with PCI BARs. Maximilian Brune wrote in #note-15: > > 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; > > 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). I proposed two different constructors in [1]. One that asserts and one that checks and allows to handle an error. I would prefer this over a separate function as it would make it clear when the check needs to be performed. If every API user with untrusted input would have to do the same `if (check(base, size)) ...`, I don't see what we win. I don't mind if somebody wants to implement it like that though. The callers that use region_create_unstrusted() in [1] are those that I identified as needing a check. [1] https://review.coreboot.org/c/coreboot/+/79905/ > > 4) Call the region well-formdness check function directly inside the SMI > > handlers before the smm_points_to_smram() check function; > > Sounds good. If they all go through smm_points_to_smram(), I would check there. Otherwise, the check could be missed on any new, future callers. That the region API is used that can overflow is also an implementation detail of smm_points_to_smram(). ---------------------------------------- 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-1753 * 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) -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
