Attention is currently required from: Felix Singer, Nico Huber, Patrick Rudolph. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56343 )
Change subject: security/lockdown: Allow to lock the controller's full address space ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
It seems (I only had a quick look at the code) that CONFIG_ROM_SIZE is taken into account. What happens if that Kconfig is not correct?
The underlying rdevs for boot_device_ro() and _rw() are defined via CONFIG_ROM_SIZE, so if it is defined incorrectly and part of the flash isn't protected, at least coreboot shouldn't be able to access those parts (the rdev API enforces range checks on everything) and thus shouldn't be vulnerable to it. spi_flash_probe() will also yell if the detected SPI part size doesn't match CONFIG_ROM_SIZE.
What happens if the bootmedia code changes and some other mechanism is used?
I mean... I think you can ask that about any piece of code anywhere. Theoretically anything could break. We just have to use good engineering practices and unit tests and whatever to try to prevent that.
When some people want features and some people want simplicity, you always have to meet somewhere in the middle. Where it's possible to cleanly isolate the more complicated features so that they can be turned off and only leave the simpler parts behind we should do that, but in this case I don't really see a better way to design it. (I'd say this patch as written doesn't fit cleanly... as I mentioned I don't think modeling this in the "protection mode" parameter makes much sense. Also, the assumption that 0x0 to 0xffffffff are valid bounds may work on the Intel controllers but if we ever have other platforms with controller-based protection, they might insist on bounds that actually match the flash range and then you have to deal with that abstraction break again.)
I see two options: 1. Just turn the LOCK_WHOLE_RO behavior into what we need; i.e. not taking the flash size into account. 2. keep our solution downstream or move it into the mainboard ports.
Yeah, I mean, like I said... if you really care I'd be okay with hacking this directly into fast_spi_flash.c (mostly because it's an x86-specific file and I don't personally care as much about those). I just don't think we should spread it out all across the code like this.