Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35349 )
Change subject: drivers/intel/fsp2_0: Allocate cfg_region_size for UPD ......................................................................
drivers/intel/fsp2_0: Allocate cfg_region_size for UPD
In FSP-S, the driver constructs its pointer to UPD using the offset in the header. The cfg_region_size should likewise be used for allocating memory and copying the default configuration. Make the change and add an error message in case there are more UPD definitions than cfg_region_size.
TEST=Verify OK on Mandolin, verify a mock error condition BUG=b:140648081
Change-Id: I20fad0e27a2ad537898b6d01e5241e1508da690c Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/silicon_init.c 1 file changed, 4 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35349/1
diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index e72e4ac..6a899bf 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -39,9 +39,11 @@ die_with_post_code(POST_INVALID_VENDOR_BINARY, "Invalid FSPS signature\n");
- upd = xmalloc(sizeof(FSPS_UPD)); + upd = xmalloc(hdr->cfg_region_size);
- memcpy(upd, supd, sizeof(FSPS_UPD)); + memcpy(upd, supd, hdr->cfg_region_size); + if (hdr->image_size < sizeof(FSPS_UPD)) + printk(BIOS_ERR, "FSP error: more UPD specified than allowed by header cfg_region_size\n");
/* Give SoC/mainboard a chance to populate entries */ platform_fsp_silicon_init_params_cb(upd);
Aaron Durbin 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 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35349/1/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/1/src/drivers/intel/fsp2_0/si... PS1, Line 42: upd = xmalloc(hdr->cfg_region_size); Add a check for cfg->region_region_size being > 0?
https://review.coreboot.org/c/coreboot/+/35349/1/src/drivers/intel/fsp2_0/si... PS1, Line 45: image_size cfg_region_size?
Hello Patrick Rudolph, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35349
to look at the new patch set (#2).
Change subject: drivers/intel/fsp2_0: Allocate cfg_region_size for UPD ......................................................................
drivers/intel/fsp2_0: Allocate cfg_region_size for UPD
In FSP-S, the driver constructs its pointer to UPD using the offset in the header. Similarly, use the header's cfg_region_size for allocating memory and copying the default configuration.
Add sanity checks for cases where there are more UPD definitions than cfg_region_size, and for a region length = 0.
TEST=Verify OK on Mandolin, verify a mock error condition BUG=b:140648081
Change-Id: I20fad0e27a2ad537898b6d01e5241e1508da690c Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/silicon_init.c 1 file changed, 10 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35349/2
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 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35349/1/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/1/src/drivers/intel/fsp2_0/si... PS1, Line 42: upd = xmalloc(hdr->cfg_region_size);
Add a check for cfg->region_region_size being > 0?
Done
https://review.coreboot.org/c/coreboot/+/35349/1/src/drivers/intel/fsp2_0/si... PS1, Line 45: image_size
cfg_region_size?
of course
Furquan Shaikh 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 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 43: (hdr->cfg_region_size) I think it should just die() if cfg_region_size is 0. I don't think it is normal to have that.
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 48: printk(BIOS_ERR, "FSP error: more UPD specified than allowed by header cfg_region_size\n"); Shouldn't this just die() here?
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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 43: (hdr->cfg_region_size)
I think it should just die() if cfg_region_size is 0. I don't think it is normal to have that.
I could go either way. The spec says that passing NULL is legal, but OTOH it says this will cause FSP to use defaults. I agree it would be a very unusual situation.
Aaron Durbin 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 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 43: (hdr->cfg_region_size)
I could go either way. […]
Passing NULL is legal, but the cfg when it is NULL should be coming from this region specifically. If it is 0 in size I'd say that's a bigger problem with expectations that we'd need to sort out in the future. Alternatively, if sizeof(FSPS_UPD) == 0 then cfg_region_size should also be 0.
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 48: printk(BIOS_ERR, "FSP error: more UPD specified than allowed by header cfg_region_size\n");
Shouldn't this just die() here?
die() is fine w/ me. Everything's undefined at this point since we don't have matching object sizes to start from.
I think we can explicitly comment that we aren't supporting running with whatever defaults were baked into the fsp blob. In practice, those defaults are typically wrong for the specific platform. We can certainly add an option in the future if that use-case is picked up (by people using bct tool to modify the blob).
Furquan Shaikh 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 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 43: (hdr->cfg_region_size)
Passing NULL is legal, but the cfg when it is NULL should be coming from this region specifically. […]
Right. Passing NULL into FSP is fine and legal. But this region size being 0 kind of breaks the rest of the assumptions here. sizeof(FSPS_UPD) being 0 is definitely an interesting case. I believe we can handle it when the need comes up?
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 48: printk(BIOS_ERR, "FSP error: more UPD specified than allowed by header cfg_region_size\n");
Everything's undefined at this point since we don't have matching object sizes to start from.
Exactly.
Hello Patrick Rudolph, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35349
to look at the new patch set (#3).
Change subject: drivers/intel/fsp2_0: Allocate cfg_region_size for UPD ......................................................................
drivers/intel/fsp2_0: Allocate cfg_region_size for UPD
In FSP-S, the driver constructs its pointer to UPD using the offset in the header. Similarly, use the header's cfg_region_size for allocating memory and copying the default configuration.
Add sanity checks for cases where there are more UPD definitions than cfg_region_size, and for a region length of 0.
TEST=Verify OK on Mandolin, verify a mock error condition BUG=b:140648081
Change-Id: I20fad0e27a2ad537898b6d01e5241e1508da690c Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/silicon_init.c 1 file changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35349/3
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/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 43: (hdr->cfg_region_size)
Right. Passing NULL into FSP is fine and legal. […]
Done
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 48: printk(BIOS_ERR, "FSP error: more UPD specified than allowed by header cfg_region_size\n");
Everything's undefined at this point since we don't have matching object sizes to start from. […]
Done
Aaron Durbin 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.
https://review.coreboot.org/c/coreboot/+/35349/3/src/drivers/intel/fsp2_0/si... PS3, Line 47: < != ??
There's some assumed behavior here that actually doesn't play out in practice for people maintaining a FSP:
1. new UPDs are added to the end preserving previous behavior of the existing fields. 2. there's no reshuffling of fields
Both are not true in practice from my experience. That's why I think we should check with !=. The memcpy() below will overlow the allocation if cfg_region_size exceeds the FSPS_UPD object. That's no good.
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))?
Aaron Durbin 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:
(1 comment)
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 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.
That is my point. They inherently need to be tightly coupled. Otherwise you can't guarantee correct behavior.
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.
While the headers and FSP blob need to be tightly coupled, that's not necessarily true in practice. I was mistaken and thinking the upd object allocation size was sizeof(FSPS_UPD). My apologies.
- 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))?
Ya. That was why I was suggesting using != for the check as a proxy for correctness. It at least deals with the < and > case, but it doesn't handle same size but reordered fields.
Hello Patrick Rudolph, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35349
to look at the new patch set (#4).
Change subject: drivers/intel/fsp2_0: Allocate cfg_region_size for UPD ......................................................................
drivers/intel/fsp2_0: Allocate cfg_region_size for UPD
In FSP-S, the driver constructs its pointer to UPD using the offset in the header. Similarly, use the header's cfg_region_size for allocating memory and copying the default configuration.
Add sanity checks for unexpedted configuration and UPD header conditions.
TEST=Verify OK on Mandolin, verify a mock error condition BUG=b:140648081
Change-Id: I20fad0e27a2ad537898b6d01e5241e1508da690c Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/silicon_init.c 1 file changed, 10 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35349/4
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 4:
(1 comment)
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 47: <
Should the second check be dropped altogether then? What's the more likely scenario, that you get […]
Done
Aaron Durbin 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 4: Code-Review+2
Furquan Shaikh 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 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35349 )
Change subject: drivers/intel/fsp2_0: Allocate cfg_region_size for UPD ......................................................................
drivers/intel/fsp2_0: Allocate cfg_region_size for UPD
In FSP-S, the driver constructs its pointer to UPD using the offset in the header. Similarly, use the header's cfg_region_size for allocating memory and copying the default configuration.
Add sanity checks for unexpedted configuration and UPD header conditions.
TEST=Verify OK on Mandolin, verify a mock error condition BUG=b:140648081
Change-Id: I20fad0e27a2ad537898b6d01e5241e1508da690c Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35349 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/drivers/intel/fsp2_0/silicon_init.c 1 file changed, 10 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index e72e4ac..ecc6e96 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -39,9 +39,17 @@ die_with_post_code(POST_INVALID_VENDOR_BINARY, "Invalid FSPS signature\n");
- upd = xmalloc(sizeof(FSPS_UPD)); + /* Disallow invalid config regions. Default settings are likely bad + * choices for coreboot, and different sized UPD from what the region + * allows is potentially a build problem. + */ + if (!hdr->cfg_region_size || hdr->cfg_region_size != sizeof(FSPS_UPD)) + die_with_post_code(POST_INVALID_VENDOR_BINARY, + "Invalid FSPS UPD region\n");
- memcpy(upd, supd, sizeof(FSPS_UPD)); + upd = xmalloc(hdr->cfg_region_size); + + memcpy(upd, supd, hdr->cfg_region_size);
/* Give SoC/mainboard a chance to populate entries */ platform_fsp_silicon_init_params_cb(upd);
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Marshall Dawson (Code Review)
-
Patrick Georgi (Code Review)