Attention is currently required from: Subrata Banik.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77176?usp=email )
Change subject: {driver, soc/intel/cmn/cse}: Refactor ISH FW Version implementation ......................................................................
Patch Set 1:
(1 comment)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/77176/comment/15e4b679_b932d829 : PS1, Line 1303: &version->cur_cse_fw_version, sizeof(struct fw_version))) {
We can't get ISH version in recovery mode as the HECI cmd that support responding to the ISH query cmd is only available in non-recovery boot.
The important part is that the CBMEM space is allocated at the same point in the boot flow with the same size, not what's in it. So if you need this to be an EARLY hook you still need to do the cbmem_add() in recovery mode, even if you then leave the space empty. (Of course if you change this to use CMOS instead you don't have to do that, which is probably the generally better option.)
There could be instanced where different callers are adding entries into the VSD at different point (like CSE version, ISH version and/or downgrade CSE etc.) and adding CRC support would requires to recalculate the CRC with addition of every new data into the VSD. We don't ned to complicate a simple DS so much.
I meant a CRC just for the ish_partition_info part, not the entire CBMEM area. I'm just saying if read random memory left over from a previous boot without any kind of validity check you may often be reading garbage.
I guess what you're actually doing here (looking through the code more closely) is checking that prev_cse_fw_version is what you expect it should be, and if it is then you assume that cur_ish_fw_version is also valid. I guess that protects against serious corruption or complete blanking of the memory. But you can still have situations where e.g. just a few bits flip (due to the memory not self-refreshing during reboot) and there happens to be a bit flip in cur_ish_fw_version but not prev_cse_fw_version. In that case you would be reporting a bogus version and a checksum could catch that. Maybe if you don't care that much about reporting an incorrect version that's not a problem.
Can you highlight what is the security implications ?
An attacker can overwrite the CBMEM region (or CMOS region, for that matter) and put whatever value they want in there to trick your firmware into misbehaving on the next boot. In general, every case where we're reading leftover data from a previous boot that's not in some kind of secure space (e.g. TPM) is a potential attack vector and we should carefully look at what could misbehave if that data was maliciously crafted.
In this case, an attacker with a temporary root exploit could just change the ISH version number in CBMEM to be whatever they want and then that fake version number would persist there across future reboots until the next time the machine loses DRAM. If this isn't used for anything other than to print a line in the firmware log that nothing depends on then I guess it's not a concern, I just wanted to make sure thought about this (and in case someone reuses that reported ISH version for some other purpose in the future they need to be aware of the same concerns).