[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linuxbios
Subject:    [coreboot] [coreboot - Bug #522] `region_overlap()` function might not work as expected due to an in
From:       Nico Huber <coreboot () fe80 ! eu>
Date:       2024-02-10 20:31:52
Message-ID: redmine.journal-1753.20240210203152.700 () fe80 ! eu
[Download RAW message or body]

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-leave@coreboot.org


[prev in list] [next in list] [prev in thread] [next in thread]