diff mbox

[1/3] fwts_cpuinfo_x86: check for null pointers when using

Message ID 1468021341-12067-2-git-send-email-ricardo.neri-calderon@linux.intel.com
State Accepted
Headers show

Commit Message

Ricardo Neri July 8, 2016, 11:42 p.m. UTC 
When successful, fwts_cpu_get_info returns a non-null pointer to a structure
containing the CPU information. However, this does not imply that all the
members of the structure are valid. This can happen if, for instance,  the
requested CPU or one of its properties was not found.

Thus, the sane thing to do is to have the callers of fwts_cpu_get_info to
make sure that data is valid before using it. Otherwise, segmentation faults
can occur if callers try to dereference NULL pointers from the CPU info
structure.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 src/bios/mtrr/mtrr.c   |  4 ++++
 src/cpu/nx/nx.c        | 16 ++++++++++++++++
 src/cpu/virt/virt.c    | 10 ++++++++++
 src/lib/src/fwts_cpu.c | 12 ++++++++++++
 4 files changed, 42 insertions(+)

Comments

Colin Ian King July 10, 2016, 6:23 p.m. UTC | #1 
On 09/07/16 00:42, Ricardo Neri wrote:
> When successful, fwts_cpu_get_info returns a non-null pointer to a structure
> containing the CPU information. However, this does not imply that all the
> members of the structure are valid. This can happen if, for instance,  the
> requested CPU or one of its properties was not found.
> 
> Thus, the sane thing to do is to have the callers of fwts_cpu_get_info to
> make sure that data is valid before using it. Otherwise, segmentation faults
> can occur if callers try to dereference NULL pointers from the CPU info
> structure.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
>  src/bios/mtrr/mtrr.c   |  4 ++++
>  src/cpu/nx/nx.c        | 16 ++++++++++++++++
>  src/cpu/virt/virt.c    | 10 ++++++++++
>  src/lib/src/fwts_cpu.c | 12 ++++++++++++
>  4 files changed, 42 insertions(+)
> 
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 3682ea1..9861fe0 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -518,6 +518,10 @@ static int mtrr_test2(fwts_framework *fw)
>  
>  static int mtrr_test3(fwts_framework *fw)
>  {
> +	if (fwts_cpuinfo->vendor_id == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU vendor_id");
> +		return FWTS_ERROR;
> +	}
>  	if (strstr(fwts_cpuinfo->vendor_id, "AMD")) {
>  		if (klog != NULL) {
>  			if (fwts_klog_regex_find(fw, klog, "SYSCFG[MtrrFixDramModEn] not cleared by BIOS, clearing this bit") > 0)
> diff --git a/src/cpu/nx/nx.c b/src/cpu/nx/nx.c
> index b930d87..f5d74b3 100644
> --- a/src/cpu/nx/nx.c
> +++ b/src/cpu/nx/nx.c
> @@ -34,6 +34,12 @@ static int nx_test1(fwts_framework *fw)
>  		return FWTS_ERROR;
>  	}
>  
> +	if (fwts_nx_cpuinfo->flags == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU flags");
> +		fwts_cpu_free_info(fwts_nx_cpuinfo);
> +		return FWTS_ERROR;
> +	}
> +
>  	if (strstr(fwts_nx_cpuinfo->flags," nx")) {
>  		fwts_passed(fw, "CPU has NX flags, BIOS is not disabling it.");
>  		fwts_cpu_free_info(fwts_nx_cpuinfo);
> @@ -105,6 +111,11 @@ static int nx_test2(fwts_framework *fw)
>  			fwts_cpu_free_info(fwts_nx_cpuinfo);
>  			return FWTS_ERROR;
>  		}
> +		if (fwts_nx_cpuinfo->flags == NULL) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "NXCPUInfoRead", "Cannot get CPU%d flags", i);
> +			fwts_cpu_free_info(fwts_nx_cpuinfo);
> +			return FWTS_ERROR;
> +		}
>  		if (i == 0) {
>  			cpu0_has_nx = (strstr(fwts_nx_cpuinfo->flags," nx") != NULL);
>  		} else {
> @@ -148,6 +159,11 @@ static int nx_test3(fwts_framework *fw)
>  			fwts_log_error(fw, "Cannot get CPU info");
>  			return FWTS_ERROR;
>  		}
> +		if (fwts_nx_cpuinfo->vendor_id == NULL) {
> +			fwts_log_error(fw, "Cannot get CPU vendor ID");
> +			fwts_cpu_free_info(fwts_nx_cpuinfo);
> +			return FWTS_ERROR;
> +		}
>  		if (strstr(fwts_nx_cpuinfo->vendor_id, "Intel") == NULL) {
>  			fwts_log_info(fw, "Non-Intel CPU, skipping test.");
>  			fwts_cpu_free_info(fwts_nx_cpuinfo);
> diff --git a/src/cpu/virt/virt.c b/src/cpu/virt/virt.c
> index f782e17..ecc84d0 100644
> --- a/src/cpu/virt/virt.c
> +++ b/src/cpu/virt/virt.c
> @@ -48,6 +48,16 @@ static int virt_init(fwts_framework *fw)
>  		return FWTS_ERROR;
>  	}
>  
> +	if (fwts_virt_cpuinfo->vendor_id == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU vendor ID");
> +		return FWTS_ERROR;
> +	}
> +
> +	if (fwts_virt_cpuinfo->flags == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU flags");
> +		return FWTS_ERROR;
> +	}
> +
>  	return FWTS_OK;
>  }
>  
> diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
> index a72500a..053f850 100644
> --- a/src/lib/src/fwts_cpu.c
> +++ b/src/lib/src/fwts_cpu.c
> @@ -171,6 +171,10 @@ static int fwts_cpu_matches_vendor_id(const char *vendor_id, bool *matches)
>  
>  	if ((cpu = fwts_cpu_get_info(0)) == NULL)
>  		return FWTS_ERROR;
> +	if (cpu->vendor_id == NULL) {
> +		fwts_cpu_free_info(cpu);
> +		return FWTS_ERROR;.
> +	}
>  
>          *matches = (strstr(cpu->vendor_id, vendor_id) != NULL);
>  
> @@ -203,6 +207,14 @@ fwts_bool fwts_cpu_has_c1e(void)
>  
>  	if ((cpu = fwts_cpu_get_info(0)) == NULL)
>  		return FWTS_BOOL_ERROR;
> +	if (cpu->flags == NULL) {
> +		rc FWTS_BOOL_ERROR;
> +		goto free_info;
> +	}
> +	if (cpu->vendor_id == NULL) {
> +		rc FWTS_BOOL_ERROR;
> +		goto free_info;
> +	}
>  
>  	/* no C1E on AMD */
>          if (strstr(cpu->vendor_id, "AuthenticAMD") == NULL) {
> 
Thanks

Acked-by: Colin Ian King <colin.king@canonical.com>
Alex Hung July 11, 2016, 2:55 a.m. UTC | #2 
On 2016-07-09 07:42 AM, Ricardo Neri wrote:
> When successful, fwts_cpu_get_info returns a non-null pointer to a structure
> containing the CPU information. However, this does not imply that all the
> members of the structure are valid. This can happen if, for instance,  the
> requested CPU or one of its properties was not found.
>
> Thus, the sane thing to do is to have the callers of fwts_cpu_get_info to
> make sure that data is valid before using it. Otherwise, segmentation faults
> can occur if callers try to dereference NULL pointers from the CPU info
> structure.
>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
>   src/bios/mtrr/mtrr.c   |  4 ++++
>   src/cpu/nx/nx.c        | 16 ++++++++++++++++
>   src/cpu/virt/virt.c    | 10 ++++++++++
>   src/lib/src/fwts_cpu.c | 12 ++++++++++++
>   4 files changed, 42 insertions(+)
>
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 3682ea1..9861fe0 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -518,6 +518,10 @@ static int mtrr_test2(fwts_framework *fw)
>
>   static int mtrr_test3(fwts_framework *fw)
>   {
> +	if (fwts_cpuinfo->vendor_id == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU vendor_id");
> +		return FWTS_ERROR;
> +	}
>   	if (strstr(fwts_cpuinfo->vendor_id, "AMD")) {
>   		if (klog != NULL) {
>   			if (fwts_klog_regex_find(fw, klog, "SYSCFG[MtrrFixDramModEn] not cleared by BIOS, clearing this bit") > 0)
> diff --git a/src/cpu/nx/nx.c b/src/cpu/nx/nx.c
> index b930d87..f5d74b3 100644
> --- a/src/cpu/nx/nx.c
> +++ b/src/cpu/nx/nx.c
> @@ -34,6 +34,12 @@ static int nx_test1(fwts_framework *fw)
>   		return FWTS_ERROR;
>   	}
>
> +	if (fwts_nx_cpuinfo->flags == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU flags");
> +		fwts_cpu_free_info(fwts_nx_cpuinfo);
> +		return FWTS_ERROR;
> +	}
> +
>   	if (strstr(fwts_nx_cpuinfo->flags," nx")) {
>   		fwts_passed(fw, "CPU has NX flags, BIOS is not disabling it.");
>   		fwts_cpu_free_info(fwts_nx_cpuinfo);
> @@ -105,6 +111,11 @@ static int nx_test2(fwts_framework *fw)
>   			fwts_cpu_free_info(fwts_nx_cpuinfo);
>   			return FWTS_ERROR;
>   		}
> +		if (fwts_nx_cpuinfo->flags == NULL) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "NXCPUInfoRead", "Cannot get CPU%d flags", i);
> +			fwts_cpu_free_info(fwts_nx_cpuinfo);
> +			return FWTS_ERROR;
> +		}
>   		if (i == 0) {
>   			cpu0_has_nx = (strstr(fwts_nx_cpuinfo->flags," nx") != NULL);
>   		} else {
> @@ -148,6 +159,11 @@ static int nx_test3(fwts_framework *fw)
>   			fwts_log_error(fw, "Cannot get CPU info");
>   			return FWTS_ERROR;
>   		}
> +		if (fwts_nx_cpuinfo->vendor_id == NULL) {
> +			fwts_log_error(fw, "Cannot get CPU vendor ID");
> +			fwts_cpu_free_info(fwts_nx_cpuinfo);
> +			return FWTS_ERROR;
> +		}
>   		if (strstr(fwts_nx_cpuinfo->vendor_id, "Intel") == NULL) {
>   			fwts_log_info(fw, "Non-Intel CPU, skipping test.");
>   			fwts_cpu_free_info(fwts_nx_cpuinfo);
> diff --git a/src/cpu/virt/virt.c b/src/cpu/virt/virt.c
> index f782e17..ecc84d0 100644
> --- a/src/cpu/virt/virt.c
> +++ b/src/cpu/virt/virt.c
> @@ -48,6 +48,16 @@ static int virt_init(fwts_framework *fw)
>   		return FWTS_ERROR;
>   	}
>
> +	if (fwts_virt_cpuinfo->vendor_id == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU vendor ID");
> +		return FWTS_ERROR;
> +	}
> +
> +	if (fwts_virt_cpuinfo->flags == NULL) {
> +		fwts_log_error(fw, "Cannot get CPU flags");
> +		return FWTS_ERROR;
> +	}
> +
>   	return FWTS_OK;
>   }
>
> diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
> index a72500a..053f850 100644
> --- a/src/lib/src/fwts_cpu.c
> +++ b/src/lib/src/fwts_cpu.c
> @@ -171,6 +171,10 @@ static int fwts_cpu_matches_vendor_id(const char *vendor_id, bool *matches)
>
>   	if ((cpu = fwts_cpu_get_info(0)) == NULL)
>   		return FWTS_ERROR;
> +	if (cpu->vendor_id == NULL) {
> +		fwts_cpu_free_info(cpu);
> +		return FWTS_ERROR;
> +	}
>
>           *matches = (strstr(cpu->vendor_id, vendor_id) != NULL);
>
> @@ -203,6 +207,14 @@ fwts_bool fwts_cpu_has_c1e(void)
>
>   	if ((cpu = fwts_cpu_get_info(0)) == NULL)
>   		return FWTS_BOOL_ERROR;
> +	if (cpu->flags == NULL) {
> +		rc FWTS_BOOL_ERROR;
> +		goto free_info;
> +	}
> +	if (cpu->vendor_id == NULL) {
> +		rc FWTS_BOOL_ERROR;
> +		goto free_info;
> +	}
>
>   	/* no C1E on AMD */
>           if (strstr(cpu->vendor_id, "AuthenticAMD") == NULL) {
>


Acked-by: Alex Hung <alex.hung@canonical.com>
diff mbox

Patch

diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
index 3682ea1..9861fe0 100644
--- a/src/bios/mtrr/mtrr.c
+++ b/src/bios/mtrr/mtrr.c
@@ -518,6 +518,10 @@  static int mtrr_test2(fwts_framework *fw)
 
 static int mtrr_test3(fwts_framework *fw)
 {
+	if (fwts_cpuinfo->vendor_id == NULL) {
+		fwts_log_error(fw, "Cannot get CPU vendor_id");
+		return FWTS_ERROR;
+	}
 	if (strstr(fwts_cpuinfo->vendor_id, "AMD")) {
 		if (klog != NULL) {
 			if (fwts_klog_regex_find(fw, klog, "SYSCFG[MtrrFixDramModEn] not cleared by BIOS, clearing this bit") > 0)
diff --git a/src/cpu/nx/nx.c b/src/cpu/nx/nx.c
index b930d87..f5d74b3 100644
--- a/src/cpu/nx/nx.c
+++ b/src/cpu/nx/nx.c
@@ -34,6 +34,12 @@  static int nx_test1(fwts_framework *fw)
 		return FWTS_ERROR;
 	}
 
+	if (fwts_nx_cpuinfo->flags == NULL) {
+		fwts_log_error(fw, "Cannot get CPU flags");
+		fwts_cpu_free_info(fwts_nx_cpuinfo);
+		return FWTS_ERROR;
+	}
+
 	if (strstr(fwts_nx_cpuinfo->flags," nx")) {
 		fwts_passed(fw, "CPU has NX flags, BIOS is not disabling it.");
 		fwts_cpu_free_info(fwts_nx_cpuinfo);
@@ -105,6 +111,11 @@  static int nx_test2(fwts_framework *fw)
 			fwts_cpu_free_info(fwts_nx_cpuinfo);
 			return FWTS_ERROR;
 		}
+		if (fwts_nx_cpuinfo->flags == NULL) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM, "NXCPUInfoRead", "Cannot get CPU%d flags", i);
+			fwts_cpu_free_info(fwts_nx_cpuinfo);
+			return FWTS_ERROR;
+		}
 		if (i == 0) {
 			cpu0_has_nx = (strstr(fwts_nx_cpuinfo->flags," nx") != NULL);
 		} else {
@@ -148,6 +159,11 @@  static int nx_test3(fwts_framework *fw)
 			fwts_log_error(fw, "Cannot get CPU info");
 			return FWTS_ERROR;
 		}
+		if (fwts_nx_cpuinfo->vendor_id == NULL) {
+			fwts_log_error(fw, "Cannot get CPU vendor ID");
+			fwts_cpu_free_info(fwts_nx_cpuinfo);
+			return FWTS_ERROR;
+		}
 		if (strstr(fwts_nx_cpuinfo->vendor_id, "Intel") == NULL) {
 			fwts_log_info(fw, "Non-Intel CPU, skipping test.");
 			fwts_cpu_free_info(fwts_nx_cpuinfo);
diff --git a/src/cpu/virt/virt.c b/src/cpu/virt/virt.c
index f782e17..ecc84d0 100644
--- a/src/cpu/virt/virt.c
+++ b/src/cpu/virt/virt.c
@@ -48,6 +48,16 @@  static int virt_init(fwts_framework *fw)
 		return FWTS_ERROR;
 	}
 
+	if (fwts_virt_cpuinfo->vendor_id == NULL) {
+		fwts_log_error(fw, "Cannot get CPU vendor ID");
+		return FWTS_ERROR;
+	}
+
+	if (fwts_virt_cpuinfo->flags == NULL) {
+		fwts_log_error(fw, "Cannot get CPU flags");
+		return FWTS_ERROR;
+	}
+
 	return FWTS_OK;
 }
 
diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
index a72500a..053f850 100644
--- a/src/lib/src/fwts_cpu.c
+++ b/src/lib/src/fwts_cpu.c
@@ -171,6 +171,10 @@  static int fwts_cpu_matches_vendor_id(const char *vendor_id, bool *matches)
 
 	if ((cpu = fwts_cpu_get_info(0)) == NULL)
 		return FWTS_ERROR;
+	if (cpu->vendor_id == NULL) {
+		fwts_cpu_free_info(cpu);
+		return FWTS_ERROR;
+	}
 
         *matches = (strstr(cpu->vendor_id, vendor_id) != NULL);
 
@@ -203,6 +207,14 @@  fwts_bool fwts_cpu_has_c1e(void)
 
 	if ((cpu = fwts_cpu_get_info(0)) == NULL)
 		return FWTS_BOOL_ERROR;
+	if (cpu->flags == NULL) {
+		rc FWTS_BOOL_ERROR;
+		goto free_info;
+	}
+	if (cpu->vendor_id == NULL) {
+		rc FWTS_BOOL_ERROR;
+		goto free_info;
+	}
 
 	/* no C1E on AMD */
         if (strstr(cpu->vendor_id, "AuthenticAMD") == NULL) {