Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(5 comments)
Hi Nate, thanks for joining the discussion.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 89: * One either has to assume that FSP doesn't perform any locking : and do it redundantly in the firmware framework, or expensively : test what is locked and keep doing so for every new FSP revision.
The direction from Intel's security team is that there have been many examples of OEM firmware that […]
Well, about the "when": How about last? Also, having it documented at least when what lock bit is set would be a step forward.
It's obvious that we got here because of somebody's agenda. But can you please get in touch with those people above you. Tell them that you complied and while it made things better for the OEMs that forgot to lock things, it made it worse for some that actually care about security.
A good example is the locking of Flash Protected Regions. Locking 0 values doesn't seem like a good solution. Now, firmware that tries to use such protection also has to test if the writes are effective (because it's not documented when FSP locks it), and lock it anyway (because it's not documented if FSP might overwrite the values). On top, tools like chipsec give you a green light for this, even if a borked configuration is locked.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 126: FSP Switches SAI : ----------------
My comment above applies for this entire section.
Though, wouldn't you agree that the first FSP-S stage is a little to early for something called POSTBOOT?
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility
To be crystal clear, FSP binaries are provided by Intel as-is. […]
Many of us don't want anything but writing our own code. It's Intel that wants us to use FSP instead, making everything ten times more expensive currently... However, I'd agree to offer Intel professional service to help fixing FSP, if they'd want to purchase a contract.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 176: None of the listed issues would exist if FSP were open-source and the : initialization steps could be integrated into coreboot, like they were : before.
To be open and direct with all of you, this is extremely unlikely.
Sorry, what do you refer to? That we could have open-source silicon init again? or the whole sentence (relation to the mentioned issues)?
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
Please take this out, otherwise I run the risk of getting in trouble for talking to you at all.
Please let's not mix this up with the removed paragraph that was talking about FSP2.1. All that this sentence says is that "if something would raise legal concerns, it should change". Not targeting anything in particular. I can drop it though, if the word "legal" alone is problematic.
This is feeling like a broken record, but I would like to point out...
- The MP_SERVICES pointer is completely optional. You can set it to NULL and FSP will use its built-in MP implementation, which negates this whole argument. I am aware of several platforms that use coreboot with this set to NULL. It is the user's choice to configure coreboot in this way, not Intel's.
Thanks for pointing this out. As mentioned somewhere above, there is a lack of documentation. It's a shame that information like this needs to be "leaked".
- The MP_SERVICES pointer is NOT part of the FSP 2.1 specification, it is an implementation level UPD that is included in the FSP for some platforms but not all.
Correct. I was told otherwise by Intel employees (that it would be part of 2.1 before that was written), but I checked and you are right.