Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47016 )
Change subject: cpu/x86/smm: Add a function for the amount of support CPUs ......................................................................
cpu/x86/smm: Add a function for the amount of support CPUs
CONFIG_MAX_CPUS might be larger than the amount for which an SMI handler is installed. So when looping over CONFIG_MAX_CPUS to fetch a SMM save state one might access memory that does not belong to a CPU node save state and get invalid results.
Change-Id: Ibc17602aa2817d910292c37d0a0095d3d9dc0aae Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/amd/smm/amd64_save_state.c M src/cpu/intel/smm/em64t100_save_state.c M src/cpu/intel/smm/em64t101_save_state.c M src/cpu/x86/smm/legacy_save_state.c M src/cpu/x86/smm/save_state.c M src/cpu/x86/smm/smihandler.c M src/cpu/x86/smm/smm_module_handler.c M src/include/cpu/x86/smm.h 8 files changed, 20 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/47016/1
diff --git a/src/cpu/amd/smm/amd64_save_state.c b/src/cpu/amd/smm/amd64_save_state.c index db329c0..a274fd1 100644 --- a/src/cpu/amd/smm/amd64_save_state.c +++ b/src/cpu/amd/smm/amd64_save_state.c @@ -84,7 +84,7 @@ int node; u8 reg_al;
- for (node = 0; node < CONFIG_MAX_CPUS; node++) { + for (node = 0; node < smm_max_cpus(); node++) { state = smm_get_save_state(node); smm_io_trap = state->smm_io_trap_offset;
diff --git a/src/cpu/intel/smm/em64t100_save_state.c b/src/cpu/intel/smm/em64t100_save_state.c index 4fd7d76..fe5f65e 100644 --- a/src/cpu/intel/smm/em64t100_save_state.c +++ b/src/cpu/intel/smm/em64t100_save_state.c @@ -72,7 +72,7 @@ em64t100_smm_state_save_area_t *state; int node;
- for (node = 0; node < CONFIG_MAX_CPUS; node++) { + for (node = 0; node < smm_max_cpus(); node++) { state = smm_get_save_state(node);
/* Check for Synchronous IO (bit0 == 1) */ diff --git a/src/cpu/intel/smm/em64t101_save_state.c b/src/cpu/intel/smm/em64t101_save_state.c index c6aa397..70112df 100644 --- a/src/cpu/intel/smm/em64t101_save_state.c +++ b/src/cpu/intel/smm/em64t101_save_state.c @@ -72,7 +72,7 @@ em64t101_smm_state_save_area_t *state; int node;
- for (node = 0; node < CONFIG_MAX_CPUS; node++) { + for (node = 0; node < smm_max_cpus(); node++) { state = smm_get_save_state(node);
/* Check for Synchronous IO (bit0 == 1) */ diff --git a/src/cpu/x86/smm/legacy_save_state.c b/src/cpu/x86/smm/legacy_save_state.c index 8e996ee..0b07155 100644 --- a/src/cpu/x86/smm/legacy_save_state.c +++ b/src/cpu/x86/smm/legacy_save_state.c @@ -71,7 +71,7 @@ legacy_smm_state_save_area_t *state; int node;
- for (node = 0; node < CONFIG_MAX_CPUS; node++) { + for (node = 0; node < smm_max_cpus(); node++) { state = smm_get_save_state(node);
/* Check AL against the requested command */ diff --git a/src/cpu/x86/smm/save_state.c b/src/cpu/x86/smm/save_state.c index bb08f86..c860b03 100644 --- a/src/cpu/x86/smm/save_state.c +++ b/src/cpu/x86/smm/save_state.c @@ -59,7 +59,7 @@ if (init_save_state()) return -1;
- if (node > CONFIG_MAX_CPUS) + if (node > smm_max_cpus()) return -1;
return save_state->get_reg(reg, node, out, length); @@ -70,7 +70,7 @@ if (init_save_state()) return -1;
- if (node > CONFIG_MAX_CPUS) + if (node > smm_max_cpus()) return -1;
return save_state->set_reg(reg, node, in, length); diff --git a/src/cpu/x86/smm/smihandler.c b/src/cpu/x86/smm/smihandler.c index 0d9131e..21b2b5e 100644 --- a/src/cpu/x86/smm/smihandler.c +++ b/src/cpu/x86/smm/smihandler.c @@ -136,6 +136,11 @@ return region_overlap(&r_smm, r); }
+uint32_t smm_max_cpus(void) +{ + return CONFIG_MAX_CPUS; +} + /** * @brief Interrupt handler for SMI# * diff --git a/src/cpu/x86/smm/smm_module_handler.c b/src/cpu/x86/smm/smm_module_handler.c index 3ba5684..89864c4 100644 --- a/src/cpu/x86/smm/smm_module_handler.c +++ b/src/cpu/x86/smm/smm_module_handler.c @@ -119,6 +119,11 @@ return region_overlap(&r_smm, r) || region_overlap(&r_aseg, r); }
+uint32_t smm_max_cpus(void) +{ + return MIN(smm_runtime->num_cpus, CONFIG_MAX_CPUS); +} + asmlinkage void smm_handler_start(void *arg) { const struct smm_module_params *p; @@ -137,7 +142,7 @@ if (smm_runtime == NULL) smm_runtime = runtime;
- if (cpu >= CONFIG_MAX_CPUS) { + if (cpu >= smm_max_cpus()) { console_init(); printk(BIOS_CRIT, "Invalid CPU number assigned in SMM stub: %d\n", cpu); diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index 6cf6f82..8bdd695 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -98,6 +98,9 @@ * account CPUs which are configured to not save their state to RAM. */ void *smm_get_save_state(int cpu);
+/* Return the amount of CPUs the smihandler is set up to handle. */ +uint32_t smm_max_cpus(void); + /* Returns true if the region overlaps with the SMM */ bool smm_region_overlaps_handler(const struct region *r);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47016 )
Change subject: cpu/x86/smm: Add a function for the amount of support CPUs ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47016/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47016/6//COMMIT_MSG@7 PS6, Line 7: support supported?
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47016?usp=email )
Change subject: cpu/x86/smm: Add a function for the amount of support CPUs ......................................................................
Abandoned
-
Arthur Heymans (Code Review) -
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)