Change in coreboot[master]: soc/intel/{broad,cannon,sky}: Fix possible out-of-bounds reads
Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34498 )
Change subject: soc/intel/{broad,cannon,sky}: Fix possible out-of-bounds reads ......................................................................
soc/intel/{broad,cannon,sky}: Fix possible out-of-bounds reads
There will be a possible out of bounds array access if power_limit_1_time == ARRAY_SIZE(power_limit_time_sec_to_msr), so prevent that in the index check. This issue was fixed for other cpus in commit 5cfef13f8d (cpu/intel: Fix out-of-bounds read due to off-by-one in condition). Based on the discussion for that commit, also remove the magic constant 28 in favour of the index of the last array element.
Change-Id: Ic3f8735b23a368f8a9395757bd52c2c40088afa1 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229673 --- M src/soc/intel/broadwell/cpu.c M src/soc/intel/cannonlake/cpu.c M src/soc/intel/skylake/cpu.c 3 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/34498/1
diff --git a/src/soc/intel/broadwell/cpu.c b/src/soc/intel/broadwell/cpu.c index af587ee..5ccaeaf 100644 --- a/src/soc/intel/broadwell/cpu.c +++ b/src/soc/intel/broadwell/cpu.c @@ -324,8 +324,8 @@ unsigned int tdp, min_power, max_power, max_time; u8 power_limit_1_val;
- if (power_limit_1_time > ARRAY_SIZE(power_limit_time_sec_to_msr)) - power_limit_1_time = 28; + if (power_limit_1_time >= ARRAY_SIZE(power_limit_time_sec_to_msr)) + power_limit_1_time = ARRAY_SIZE(power_limit_time_sec_to_msr) - 1;
if (!(msr.lo & PLATFORM_INFO_SET_TDP)) return; diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 7eb413c..b0eaa5d 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -108,8 +108,8 @@
config_t *conf = config_of_path(SA_DEVFN_ROOT);
- if (power_limit_1_time > ARRAY_SIZE(power_limit_time_sec_to_msr)) - power_limit_1_time = 28; + if (power_limit_1_time >= ARRAY_SIZE(power_limit_time_sec_to_msr)) + power_limit_1_time = ARRAY_SIZE(power_limit_time_sec_to_msr) - 1;
if (!(msr.lo & PLATFORM_INFO_SET_TDP)) return; diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index 2fd01b4..cb0ceaa 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -119,8 +119,8 @@
config_t *conf = config_of_path(SA_DEVFN_ROOT);
- if (power_limit_1_time > ARRAY_SIZE(power_limit_time_sec_to_msr)) - power_limit_1_time = 28; + if (power_limit_1_time >= ARRAY_SIZE(power_limit_time_sec_to_msr)) + power_limit_1_time = ARRAY_SIZE(power_limit_time_sec_to_msr) - 1;
if (!(msr.lo & PLATFORM_INFO_SET_TDP)) return;
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34498 )
Change subject: soc/intel/{broad,cannon,sky}: Fix possible out-of-bounds reads ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34498 )
Change subject: soc/intel/{broad,cannon,sky}: Fix possible out-of-bounds reads ......................................................................
soc/intel/{broad,cannon,sky}: Fix possible out-of-bounds reads
There will be a possible out of bounds array access if power_limit_1_time == ARRAY_SIZE(power_limit_time_sec_to_msr), so prevent that in the index check. This issue was fixed for other cpus in commit 5cfef13f8d (cpu/intel: Fix out-of-bounds read due to off-by-one in condition). Based on the discussion for that commit, also remove the magic constant 28 in favour of the index of the last array element.
Change-Id: Ic3f8735b23a368f8a9395757bd52c2c40088afa1 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229673 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34498 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/soc/intel/broadwell/cpu.c M src/soc/intel/cannonlake/cpu.c M src/soc/intel/skylake/cpu.c 3 files changed, 6 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/soc/intel/broadwell/cpu.c b/src/soc/intel/broadwell/cpu.c index af587ee..5ccaeaf 100644 --- a/src/soc/intel/broadwell/cpu.c +++ b/src/soc/intel/broadwell/cpu.c @@ -324,8 +324,8 @@ unsigned int tdp, min_power, max_power, max_time; u8 power_limit_1_val;
- if (power_limit_1_time > ARRAY_SIZE(power_limit_time_sec_to_msr)) - power_limit_1_time = 28; + if (power_limit_1_time >= ARRAY_SIZE(power_limit_time_sec_to_msr)) + power_limit_1_time = ARRAY_SIZE(power_limit_time_sec_to_msr) - 1;
if (!(msr.lo & PLATFORM_INFO_SET_TDP)) return; diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 7eb413c..b0eaa5d 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -108,8 +108,8 @@
config_t *conf = config_of_path(SA_DEVFN_ROOT);
- if (power_limit_1_time > ARRAY_SIZE(power_limit_time_sec_to_msr)) - power_limit_1_time = 28; + if (power_limit_1_time >= ARRAY_SIZE(power_limit_time_sec_to_msr)) + power_limit_1_time = ARRAY_SIZE(power_limit_time_sec_to_msr) - 1;
if (!(msr.lo & PLATFORM_INFO_SET_TDP)) return; diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index 2fd01b4..cb0ceaa 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -119,8 +119,8 @@
config_t *conf = config_of_path(SA_DEVFN_ROOT);
- if (power_limit_1_time > ARRAY_SIZE(power_limit_time_sec_to_msr)) - power_limit_1_time = 28; + if (power_limit_1_time >= ARRAY_SIZE(power_limit_time_sec_to_msr)) + power_limit_1_time = ARRAY_SIZE(power_limit_time_sec_to_msr) - 1;
if (!(msr.lo & PLATFORM_INFO_SET_TDP)) return;
-
Jacob Garber (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)