Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41086 )
Change subject: SMM: Validate more user-provided pointers ......................................................................
Patch Set 18:
(23 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? […]
I think we have better ways to get a PCI base address, but that's for another patch
https://review.coreboot.org/c/coreboot/+/41086/17/src/mainboard/lenovo/t60/s... PS17, Line 27: *(bar+LVTMA_BL_MOD_LEVEL)
This should really be using read8/write8 functions
Not trivial to do with the operations below, better to handle in a separate patch
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... File src/soc/intel/baytrail/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 7: #include <commonlib/region.h>
Oh, I thought smm_points_to_smram was in this header. […]
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/baytrail/smi... PS12, Line 337: break
It should definitely return. […]
Done
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...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/baytrail/smi... PS17, Line 331: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/11/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/11/src/soc/intel/braswell/smi... PS11, Line 306: return;
Hmm.. we seem to call smm_setup_structures() late in ramstage, and reach here. […]
Ack
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/braswell/smi... File src/soc/intel/braswell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/braswell/smi... PS12, Line 314: break
should this return instead?
Done
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...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/braswell/smi... PS17, Line 307: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/broadwell/sm... File src/soc/intel/broadwell/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/broadwell/sm... PS12, Line 361: break
return?
Done
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...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/broadwell/sm... PS17, Line 351: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/common/block... File src/soc/intel/common/block/smm/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/soc/intel/common/block... PS12, Line 387: break
return?
Done
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 7: #include <commonlib/region.h>
I don't see any new calls to any region or rdev functions...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/soc/intel/common/block... PS17, Line 379: break
return
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/bd82... PS12, Line 113: if (!smm_points_to_smram((void *)(uintptr_t)xhci_bar,
I think it may be safer to do this check at the top of the case statement. […]
The conditions are always false. Added a FIXME comment.
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...
Done
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)); : }
I'd just drop this, it's broken
Added a comment for now, the conditions are always false
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...
Done
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/lynx... File src/southbridge/intel/lynxpoint/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41086/12/src/southbridge/intel/lynx... PS12, Line 332: break
return?
Done
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...
Done
https://review.coreboot.org/c/coreboot/+/41086/17/src/southbridge/intel/lynx... PS17, Line 323: break
return
Done