Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, cedarhouse1@comcast.net,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38763
to review the following change.
Change subject: cpu/x86/smm: Add overflow check ......................................................................
cpu/x86/smm: Add overflow check
Rather bail out than run into undefined behavior.
Change-Id: Ife26a0abed0ce6bcafe1e7cd8f499618631c4df4 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/cpu/x86/smm/smm_module_loader.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/38763/1
diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index a421436..81020a4 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -202,6 +202,8 @@ /* Adjust remaining size to account for save state. */ total_save_state_size = params->per_cpu_save_state_size * params->num_concurrent_save_states; + if (total_save_state_size > size) + return -1; size -= total_save_state_size;
/* The save state size encroached over the first SMM entry point. */
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38763 )
Change subject: cpu/x86/smm: Add overflow check ......................................................................
Patch Set 1:
I am not sure if rest of the code survives if SMM stub is left uninstalled. I decided to abandon x86-smm-tseg topic now, but there are some static uses of IS_ENABLED(HAVE_SMI_HANDLER) that might take bad route. Maybe even die()?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38763 )
Change subject: cpu/x86/smm: Add overflow check ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38763 )
Change subject: cpu/x86/smm: Add overflow check ......................................................................
Patch Set 1: Code-Review+2
cedarhouse1@comcast.net has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38763 )
Change subject: cpu/x86/smm: Add overflow check ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38763 )
Change subject: cpu/x86/smm: Add overflow check ......................................................................
Patch Set 1:
I am not sure if rest of the code survives if SMM stub is left uninstalled. I decided to abandon x86-smm-tseg topic now, but there are some static uses of IS_ENABLED(HAVE_SMI_HANDLER) that might take bad route. Maybe even die()?
Not sure either, but I'm not going to test all platforms that use this code. Also, the function is full of `return -1;` in the error paths, so that's up to the caller.
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38763 )
Change subject: cpu/x86/smm: Add overflow check ......................................................................
cpu/x86/smm: Add overflow check
Rather bail out than run into undefined behavior.
Change-Id: Ife26a0abed0ce6bcafe1e7cd8f499618631c4df4 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/38763 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: cedarhouse1@comcast.net --- M src/cpu/x86/smm/smm_module_loader.c 1 file changed, 2 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, approved cedarhouse1@comcast.net: Looks good to me, but someone else must approve
diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index a421436..81020a4 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -202,6 +202,8 @@ /* Adjust remaining size to account for save state. */ total_save_state_size = params->per_cpu_save_state_size * params->num_concurrent_save_states; + if (total_save_state_size > size) + return -1; size -= total_save_state_size;
/* The save state size encroached over the first SMM entry point. */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38763 )
Change subject: cpu/x86/smm: Add overflow check ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/514 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/513 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/512
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Kyösti Mälkki (Code Review)
-
Name of user not set (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)