Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 17:
(13 comments)
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... File src/mainboard/lenovo/t60/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... PS17, Line 20: ~0xf What about a #define for this (very common) mask? #define PCI_BASE_ADDRESS_MASK ~0xF something like that in pci.h ?
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... PS17, Line 331: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... PS17, Line 8: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... PS17, Line 307: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... File src/soc/intel/broadwell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... PS17, Line 9: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... PS17, Line 351: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... PS17, Line 379: break return
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/bd82... PS17, Line 107: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar, : 0x4F0 + sizeof(uint32_t))) { : /* FIXME: This looks broken */ : if ((xhci_bar + 0x4C0) & 1) : pch_iobp_update(0xEC000082, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4D0) & 1) : pch_iobp_update(0xEC000182, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4E0) & 1) : pch_iobp_update(0xEC000282, ~0UL, (3 << 2)); : if ((xhci_bar + 0x4F0) & 1) : pch_iobp_update(0xEC000382, ~0UL, (3 << 2)); : } Would prefer if this just return'ed instead of conditionally executing these. If the pointers overlap, something is broken and very wrong, or there's a rootkit.
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/ibex... File src/southbridge/intel/ibexpeak/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/ibex... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... File src/southbridge/intel/lynxpoint/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... PS17, Line 7: #include <commonlib/region.h> I don't see any new calls to any region or rdev functions...
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... PS17, Line 323: break return