Attention is currently required from: Arthur Heymans, Raul Rangel, Nico Huber, Angel Pons, Martin Roth, Karthik Ramasubramanian, Felix Held.
Robert Zieba has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67931 )
Change subject: cpu/x86/smm: Add PCI BAR store functionality ......................................................................
Patch Set 7:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/67931/comment/7f09e16a_92ed77f1 PS2, Line 9: n certain cases data within protected memmory areas like SMRAM could : be leaked or modified if an attacker remaps PCI BARs to point within : that area.
Thanks Nico, great explanation.
Done
https://review.coreboot.org/c/coreboot/+/67931/comment/f3508fa0_a62339b9 PS2, Line 11: This commit adds support to the existing SMM to allow storing : PCI BARs in SMRAM and then later retrieved.
Done in the sense that it seems like the original question has been answered between this comment ht […]
Done
File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/67931/comment/a1f27bdd_049a8f4b PS5, Line 195: : config SMM_PCI_BAR_STORE : bool : depends on SMM_CHIPSET_STORE : help : This option enables support for storing PCI BARs in SMRAM so they : can't be tampered with. : : config SMM_PCI_BAR_STORE_NUM_SLOTS : int : default 8 : help : Number of slots available to store PCI BARs in SMRAM
Robert, that makes sense to me. […]
Done
File src/cpu/x86/smm/pci_bar_store.c:
https://review.coreboot.org/c/coreboot/+/67931/comment/b03372c6_0b38a3eb PS5, Line 7: smm_pci_query_stored_bar
Raul, you're right that we don't in this case, but is there an argument against making it generic?
Removed this particular function as it wasn't as useful as I initially thought.
https://review.coreboot.org/c/coreboot/+/67931/comment/d19f3b99_bcd73484 PS5, Line 30: pci_devfn_t pci_addr,
Definitely seems reasonable.
Done
https://review.coreboot.org/c/coreboot/+/67931/comment/97e9817b_1e3ef8aa PS5, Line 32: uint32_t bars[] = { : PCI_BASE_ADDRESS_0, : PCI_BASE_ADDRESS_1, : PCI_BASE_ADDRESS_2, : PCI_BASE_ADDRESS_3, : PCI_BASE_ADDRESS_4, : PCI_BASE_ADDRESS_5, : };
This is only true for PCI_HEADER_TYPE_NORMAL. […]
Done
https://review.coreboot.org/c/coreboot/+/67931/comment/4faee21c_c3491b48 PS5, Line 58: smm_mainboard_pci_bar_store_init(&smm_runtime->chipset_store.pci_bar_store[0], : CONFIG_SMM_PCI_BAR_STORE_NUM_SLOTS);
Also a good suggestion.
Done
File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/67931/comment/a2c64258_36cd75ab PS5, Line 71: struct smm_chipset_store { : #if CONFIG(SMM_PCI_BAR_STORE) : struct smm_pci_bar_info pci_bar_store[CONFIG_SMM_PCI_BAR_STORE_NUM_SLOTS]; : #endif : };
I would imagine this gets defined by the chipset. The only change to this code would be: […]
Removed chipset specific struct.
https://review.coreboot.org/c/coreboot/+/67931/comment/aa7868b3_d00512b2 PS5, Line 213: smm_get_runtime
Make this `smm_chipset_store`. We don't want to expose all the runtime data if we don't need it.
Done
https://review.coreboot.org/c/coreboot/+/67931/comment/a59e8f27_06f74c96 PS5, Line 215: /* Returns the previously stored BAR for the given PCI address or 0 on failure. */ : uintptr_t smm_pci_query_stored_bar(pci_devfn_t pci_addr, uint8_t index); : void smm_pci_get_stored_bars(const volatile struct smm_pci_bar_info **out_bar_store, : size_t *out_size); : /* Weak handler function to store PCI BARs. */ : void smm_mainboard_pci_bar_store_init(struct smm_pci_bar_info *bar_store, size_t size); : /* Helper function to fill a BAR store entry. */ : void smm_pci_bar_store_fill_entry(pci_devfn_t pci_addr, struct smm_pci_bar_info *entry);
No need for this here.
I think this is resolved by getting rid of the chipset specific stuff. Let me know if I'm misunderstanding.
https://review.coreboot.org/c/coreboot/+/67931/comment/d956de8a_58653c79 PS5, Line 225: smm_runtime
smm_chipset_store
I'm fine with either approach, but this way allows the SMM init code to call this function without having to wrap it in an `#ifdef` block.
https://review.coreboot.org/c/coreboot/+/67931/comment/8769a78a_8fbe1038 PS5, Line 225: smm_pci_bar_store_init
`smm_chipset_store_init`
Resolved with the removal of chipset specific data.
-
Robert Zieba (Code Review)