Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35349 )
Change subject: drivers/intel/fsp2_0: Allocate cfg_region_size for UPD ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35349/3/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/3/src/drivers/intel/fsp2_0/si... PS3, Line 32: FSPS_UPD *upd = NULL;
This change can be dropped.
Ack
https://review.coreboot.org/c/coreboot/+/35349/3/src/drivers/intel/fsp2_0/si... PS3, Line 47: < Should the second check be dropped altogether then? What's the more likely scenario, that you get a new image with more UPDs than coreboot has definitions for, or a newer coreboot w/older FSP image? If this changes to != then that means FSP and the include file must always be in sync.
The memcpy() below will overlow the allocation if cfg_region_size exceeds the FSPS_UPD object. That's no good.
Maybe I missed the scenario you're describing. We're copying out of the FSP blob into memory, and only sizeof(region FSP says it has). Same as allocated. * Hmm, here's a related scenario though. If sizeof(FSPS_UPD) is larger than what we allocated, then platform_fsp_silicon_init_params_cb() could easily write locations outside of the allocated region. Maybe allocate max(cfg_region_size, sizeof(FSPS_UPD))?