opal: add more bounds checking and zero pstates array
diff mbox series

Message ID 20190829070530.21397-1-colin.king@canonical.com
State Accepted
Headers show
Series
  • opal: add more bounds checking and zero pstates array
Related show

Commit Message

Colin King Aug. 29, 2019, 7:05 a.m. UTC
From: Colin Ian King <colin.king@canonical.com>

Currently there is no bounds checking on the maximum number
of pstates, so we can potentially have an out-of-bounds read
on the pstates array. Fix this by checking and force setting
maximum bounds.  Also zero the pstates array to ensure no
uninitialized pstates values are printed.  Finally add some
missing spaces in some warning messages.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/opal/power_mgmt_info.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Alex Hung Sept. 16, 2019, 7:52 a.m. UTC | #1
On 2019-08-29 9:05 a.m., Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently there is no bounds checking on the maximum number
> of pstates, so we can potentially have an out-of-bounds read
> on the pstates array. Fix this by checking and force setting
> maximum bounds.  Also zero the pstates array to ensure no
> uninitialized pstates values are printed.  Finally add some
> missing spaces in some warning messages.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/opal/power_mgmt_info.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/opal/power_mgmt_info.c b/src/opal/power_mgmt_info.c
> index ab90b9f2..1175278c 100644
> --- a/src/opal/power_mgmt_info.c
> +++ b/src/opal/power_mgmt_info.c
> @@ -152,6 +152,7 @@ static int pstate_limits_test(fwts_framework *fw)
>  		return FWTS_ERROR;
>  	}
>  
> +	memset(pstates, 0, sizeof(pstates));
>  	ret = fwts_dt_property_read_u32_arr(fw->fdt, offset, "ibm,pstate-ids",
>  					pstates, &len);
>  	if (ret != FWTS_OK) {
> @@ -179,12 +180,12 @@ static int pstate_limits_test(fwts_framework *fw)
>  
>  	if (proc_gen == proc_gen_p8 && nr_pstates > 128)
>  		fwts_log_warning(fw,
> -				"More than 128 pstates found,nr_pstates = %d",
> +				"More than 128 pstates found, nr_pstates = %d",
>  				 nr_pstates);
>  
>  	if (proc_gen == proc_gen_p9 && nr_pstates > 255)
>  		fwts_log_warning(fw,
> -				"More than 255 pstates found,nr_pstates = %d",
> +				"More than 255 pstates found, nr_pstates = %d",
>  				 nr_pstates);
>  
>  	if (len != nr_pstates)
> @@ -192,6 +193,12 @@ static int pstate_limits_test(fwts_framework *fw)
>  				"Expected %d pstates, found %d pstates",
>  				nr_pstates, len);
>  
> +	/* Avoid reads outside of pstates[] */
> +	if (nr_pstates > MAX_PSTATES) {
> +		nr_pstates = MAX_PSTATES;
> +		fwts_log_warning(fw, "Truncating to %d pstates\n", nr_pstates);
> +	}
> +
>  	for (i = 0; i < nr_pstates; i++) {
>  		if (cmp_pstates(pstate_max, pstates[i]) < 0) {
>  			fwts_log_warning(fw, "Invalid Pstate id %d "
> 
Acked-by: Alex Hung <alex.hung@canonical.com>
ivanhu Sept. 20, 2019, 12:57 p.m. UTC | #2
On 8/29/19 3:05 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently there is no bounds checking on the maximum number
> of pstates, so we can potentially have an out-of-bounds read
> on the pstates array. Fix this by checking and force setting
> maximum bounds.  Also zero the pstates array to ensure no
> uninitialized pstates values are printed.  Finally add some
> missing spaces in some warning messages.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/opal/power_mgmt_info.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/opal/power_mgmt_info.c b/src/opal/power_mgmt_info.c
> index ab90b9f2..1175278c 100644
> --- a/src/opal/power_mgmt_info.c
> +++ b/src/opal/power_mgmt_info.c
> @@ -152,6 +152,7 @@ static int pstate_limits_test(fwts_framework *fw)
>  		return FWTS_ERROR;
>  	}
>  
> +	memset(pstates, 0, sizeof(pstates));
>  	ret = fwts_dt_property_read_u32_arr(fw->fdt, offset, "ibm,pstate-ids",
>  					pstates, &len);
>  	if (ret != FWTS_OK) {
> @@ -179,12 +180,12 @@ static int pstate_limits_test(fwts_framework *fw)
>  
>  	if (proc_gen == proc_gen_p8 && nr_pstates > 128)
>  		fwts_log_warning(fw,
> -				"More than 128 pstates found,nr_pstates = %d",
> +				"More than 128 pstates found, nr_pstates = %d",
>  				 nr_pstates);
>  
>  	if (proc_gen == proc_gen_p9 && nr_pstates > 255)
>  		fwts_log_warning(fw,
> -				"More than 255 pstates found,nr_pstates = %d",
> +				"More than 255 pstates found, nr_pstates = %d",
>  				 nr_pstates);
>  
>  	if (len != nr_pstates)
> @@ -192,6 +193,12 @@ static int pstate_limits_test(fwts_framework *fw)
>  				"Expected %d pstates, found %d pstates",
>  				nr_pstates, len);
>  
> +	/* Avoid reads outside of pstates[] */
> +	if (nr_pstates > MAX_PSTATES) {
> +		nr_pstates = MAX_PSTATES;
> +		fwts_log_warning(fw, "Truncating to %d pstates\n", nr_pstates);
> +	}
> +
>  	for (i = 0; i < nr_pstates; i++) {
>  		if (cmp_pstates(pstate_max, pstates[i]) < 0) {
>  			fwts_log_warning(fw, "Invalid Pstate id %d "


Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch
diff mbox series

diff --git a/src/opal/power_mgmt_info.c b/src/opal/power_mgmt_info.c
index ab90b9f2..1175278c 100644
--- a/src/opal/power_mgmt_info.c
+++ b/src/opal/power_mgmt_info.c
@@ -152,6 +152,7 @@  static int pstate_limits_test(fwts_framework *fw)
 		return FWTS_ERROR;
 	}
 
+	memset(pstates, 0, sizeof(pstates));
 	ret = fwts_dt_property_read_u32_arr(fw->fdt, offset, "ibm,pstate-ids",
 					pstates, &len);
 	if (ret != FWTS_OK) {
@@ -179,12 +180,12 @@  static int pstate_limits_test(fwts_framework *fw)
 
 	if (proc_gen == proc_gen_p8 && nr_pstates > 128)
 		fwts_log_warning(fw,
-				"More than 128 pstates found,nr_pstates = %d",
+				"More than 128 pstates found, nr_pstates = %d",
 				 nr_pstates);
 
 	if (proc_gen == proc_gen_p9 && nr_pstates > 255)
 		fwts_log_warning(fw,
-				"More than 255 pstates found,nr_pstates = %d",
+				"More than 255 pstates found, nr_pstates = %d",
 				 nr_pstates);
 
 	if (len != nr_pstates)
@@ -192,6 +193,12 @@  static int pstate_limits_test(fwts_framework *fw)
 				"Expected %d pstates, found %d pstates",
 				nr_pstates, len);
 
+	/* Avoid reads outside of pstates[] */
+	if (nr_pstates > MAX_PSTATES) {
+		nr_pstates = MAX_PSTATES;
+		fwts_log_warning(fw, "Truncating to %d pstates\n", nr_pstates);
+	}
+
 	for (i = 0; i < nr_pstates; i++) {
 		if (cmp_pstates(pstate_max, pstates[i]) < 0) {
 			fwts_log_warning(fw, "Invalid Pstate id %d "