diff mbox

uefivarinfo: allocate buffer rewrite to avoid realloc failure (LP: #1362540)

Message ID 1410762848-18216-1-git-send-email-ivan.hu@canonical.com
State Accepted
Headers show

Commit Message

Ivan Hu Sept. 15, 2014, 6:34 a.m. UTC 
The data buffer which was brought into firmware seems to be changed that caused
the realloc function failed below. Rewrite the function that allocate buffer
with the max allowed variable size without reallocing the memory.

*** glibc detected *** fwts: realloc(): invalid next size: 0x00007f5e807a7080 **

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 src/uefi/uefivarinfo/uefivarinfo.c |   55 +++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 26 deletions(-)

Comments

Colin Ian King Sept. 15, 2014, 12:24 p.m. UTC | #1 
On 15/09/14 07:34, Ivan Hu wrote:
> The data buffer which was brought into firmware seems to be changed that caused
> the realloc function failed below. Rewrite the function that allocate buffer
> with the max allowed variable size without reallocing the memory.
> 
> *** glibc detected *** fwts: realloc(): invalid next size: 0x00007f5e807a7080 **
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/uefi/uefivarinfo/uefivarinfo.c |   55 +++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/src/uefi/uefivarinfo/uefivarinfo.c b/src/uefi/uefivarinfo/uefivarinfo.c
> index 84e960b..a4d2783 100644
> --- a/src/uefi/uefivarinfo/uefivarinfo.c
> +++ b/src/uefi/uefivarinfo/uefivarinfo.c
> @@ -29,7 +29,6 @@
>  #include "fwts_efi_module.h"
>  
>  #define MAX_VARNAME_LENGTH	1024
> -#define DATA_LENGTH		1024
>  
>  static int fd;
>  
> @@ -66,7 +65,7 @@ static int do_checkvariables(
>  	fwts_framework *fw,
>  	uint64_t *usedvars,
>  	uint64_t *usedvarssize,
> -	uint64_t maxvarsize)
> +	const uint64_t maxvarsize)
>  {
>  	long ioret;
>  	uint64_t status;
> @@ -114,46 +113,50 @@ static int do_checkvariables(
>  
>  		(*usedvars)++;
>  
> -		data = malloc(DATA_LENGTH);
> +		data = malloc(maxvarsize);
>  		if (!data) {
>  			fwts_log_info(fw, "Failed to allocate memory for test.");
>  			return FWTS_ERROR;
>  		}
>  
> -		getdatasize = DATA_LENGTH;
> +		getdatasize = maxvarsize;
>  		getvariable.VariableName = variablename;
>  		getvariable.VendorGuid = &vendorguid;
>  		getvariable.DataSize = &getdatasize;
>  		getvariable.Data = data;
> -		while (true) {
> -			ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> -			if (ioret == -1) {
> -				if (status != EFI_BUFFER_TOO_SMALL) {
> -					fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
> -					fwts_uefi_print_status_info(fw, status);
> -					free(data);
> +
> +		ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> +		if (ioret == -1) {
> +			free(data);
> +			if (status != EFI_BUFFER_TOO_SMALL) {
> +				fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
> +				fwts_uefi_print_status_info(fw, status);
> +				return FWTS_ERROR;
> +			} else if (getdatasize > maxvarsize) {
> +				fwts_log_info(fw, "Variable is larger than maximum variable length.");
> +				fwts_uefi_print_status_info(fw, status);
> +
> +				/*
> +				 * Although the variable is larger than maximum variable length,
> +				 * still try to calculate the total sizes of the used variables.
> +				 */
> +				data = malloc(getdatasize);
> +				if (!data) {
> +					fwts_log_info(fw, "Failed to allocate memory for test.");
>  					return FWTS_ERROR;
>  				}
> -				if (getdatasize == maxvarsize) {
> -					fwts_log_info(fw, "Variable is larger than maximum variable length.");
> +
> +				getvariable.Data = data;
> +				ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> +				if (ioret == -1) {
> +					fwts_log_info(fw, "Failed to get variable with variable larger than maximum variable length.");
>  					fwts_uefi_print_status_info(fw, status);
>  					free(data);
>  					return FWTS_ERROR;
>  				}
> -				getdatasize += DATA_LENGTH;
> -				getdatasize = (getdatasize > maxvarsize) ? maxvarsize : getdatasize;
> -				data = realloc(data, getdatasize);
> -				if (data) {
> -					getvariable.DataSize = &getdatasize;
> -					getvariable.Data = data;
> -					continue;
> -				} else {
> -					fwts_log_info(fw, "Failed to allocate memory for test.");
> -					return FWTS_ERROR;
> -				}
>  			}
> -			break;
> -		};
> +		}
> +
>  		free(data);
>  
>  		(*usedvarssize) += getdatasize;
> 
Looks very reasonable to me, I've not tested it though.

Acked-by: Colin Ian King <colin.king@canonical.com>
Alex Hung Sept. 16, 2014, 2:20 a.m. UTC | #2 
On 14-09-15 02:34 PM, Ivan Hu wrote:
> The data buffer which was brought into firmware seems to be changed that caused
> the realloc function failed below. Rewrite the function that allocate buffer
> with the max allowed variable size without reallocing the memory.
>
> *** glibc detected *** fwts: realloc(): invalid next size: 0x00007f5e807a7080 **
>
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>   src/uefi/uefivarinfo/uefivarinfo.c |   55 +++++++++++++++++++-----------------
>   1 file changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/src/uefi/uefivarinfo/uefivarinfo.c b/src/uefi/uefivarinfo/uefivarinfo.c
> index 84e960b..a4d2783 100644
> --- a/src/uefi/uefivarinfo/uefivarinfo.c
> +++ b/src/uefi/uefivarinfo/uefivarinfo.c
> @@ -29,7 +29,6 @@
>   #include "fwts_efi_module.h"
>   
>   #define MAX_VARNAME_LENGTH	1024
> -#define DATA_LENGTH		1024
>   
>   static int fd;
>   
> @@ -66,7 +65,7 @@ static int do_checkvariables(
>   	fwts_framework *fw,
>   	uint64_t *usedvars,
>   	uint64_t *usedvarssize,
> -	uint64_t maxvarsize)
> +	const uint64_t maxvarsize)
>   {
>   	long ioret;
>   	uint64_t status;
> @@ -114,46 +113,50 @@ static int do_checkvariables(
>   
>   		(*usedvars)++;
>   
> -		data = malloc(DATA_LENGTH);
> +		data = malloc(maxvarsize);
>   		if (!data) {
>   			fwts_log_info(fw, "Failed to allocate memory for test.");
>   			return FWTS_ERROR;
>   		}
>   
> -		getdatasize = DATA_LENGTH;
> +		getdatasize = maxvarsize;
>   		getvariable.VariableName = variablename;
>   		getvariable.VendorGuid = &vendorguid;
>   		getvariable.DataSize = &getdatasize;
>   		getvariable.Data = data;
> -		while (true) {
> -			ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> -			if (ioret == -1) {
> -				if (status != EFI_BUFFER_TOO_SMALL) {
> -					fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
> -					fwts_uefi_print_status_info(fw, status);
> -					free(data);
> +
> +		ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> +		if (ioret == -1) {
> +			free(data);
> +			if (status != EFI_BUFFER_TOO_SMALL) {
> +				fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
> +				fwts_uefi_print_status_info(fw, status);
> +				return FWTS_ERROR;
> +			} else if (getdatasize > maxvarsize) {
> +				fwts_log_info(fw, "Variable is larger than maximum variable length.");
> +				fwts_uefi_print_status_info(fw, status);
> +
> +				/*
> +				 * Although the variable is larger than maximum variable length,
> +				 * still try to calculate the total sizes of the used variables.
> +				 */
> +				data = malloc(getdatasize);
> +				if (!data) {
> +					fwts_log_info(fw, "Failed to allocate memory for test.");
>   					return FWTS_ERROR;
>   				}
> -				if (getdatasize == maxvarsize) {
> -					fwts_log_info(fw, "Variable is larger than maximum variable length.");
> +
> +				getvariable.Data = data;
> +				ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> +				if (ioret == -1) {
> +					fwts_log_info(fw, "Failed to get variable with variable larger than maximum variable length.");
>   					fwts_uefi_print_status_info(fw, status);
>   					free(data);
>   					return FWTS_ERROR;
>   				}
> -				getdatasize += DATA_LENGTH;
> -				getdatasize = (getdatasize > maxvarsize) ? maxvarsize : getdatasize;
> -				data = realloc(data, getdatasize);
> -				if (data) {
> -					getvariable.DataSize = &getdatasize;
> -					getvariable.Data = data;
> -					continue;
> -				} else {
> -					fwts_log_info(fw, "Failed to allocate memory for test.");
> -					return FWTS_ERROR;
> -				}
>   			}
> -			break;
> -		};
> +		}
> +
>   		free(data);
>   
>   		(*usedvarssize) += getdatasize;

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

Patch

diff --git a/src/uefi/uefivarinfo/uefivarinfo.c b/src/uefi/uefivarinfo/uefivarinfo.c
index 84e960b..a4d2783 100644
--- a/src/uefi/uefivarinfo/uefivarinfo.c
+++ b/src/uefi/uefivarinfo/uefivarinfo.c
@@ -29,7 +29,6 @@ 
 #include "fwts_efi_module.h"
 
 #define MAX_VARNAME_LENGTH	1024
-#define DATA_LENGTH		1024
 
 static int fd;
 
@@ -66,7 +65,7 @@  static int do_checkvariables(
 	fwts_framework *fw,
 	uint64_t *usedvars,
 	uint64_t *usedvarssize,
-	uint64_t maxvarsize)
+	const uint64_t maxvarsize)
 {
 	long ioret;
 	uint64_t status;
@@ -114,46 +113,50 @@  static int do_checkvariables(
 
 		(*usedvars)++;
 
-		data = malloc(DATA_LENGTH);
+		data = malloc(maxvarsize);
 		if (!data) {
 			fwts_log_info(fw, "Failed to allocate memory for test.");
 			return FWTS_ERROR;
 		}
 
-		getdatasize = DATA_LENGTH;
+		getdatasize = maxvarsize;
 		getvariable.VariableName = variablename;
 		getvariable.VendorGuid = &vendorguid;
 		getvariable.DataSize = &getdatasize;
 		getvariable.Data = data;
-		while (true) {
-			ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
-			if (ioret == -1) {
-				if (status != EFI_BUFFER_TOO_SMALL) {
-					fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
-					fwts_uefi_print_status_info(fw, status);
-					free(data);
+
+		ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
+		if (ioret == -1) {
+			free(data);
+			if (status != EFI_BUFFER_TOO_SMALL) {
+				fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
+				fwts_uefi_print_status_info(fw, status);
+				return FWTS_ERROR;
+			} else if (getdatasize > maxvarsize) {
+				fwts_log_info(fw, "Variable is larger than maximum variable length.");
+				fwts_uefi_print_status_info(fw, status);
+
+				/*
+				 * Although the variable is larger than maximum variable length,
+				 * still try to calculate the total sizes of the used variables.
+				 */
+				data = malloc(getdatasize);
+				if (!data) {
+					fwts_log_info(fw, "Failed to allocate memory for test.");
 					return FWTS_ERROR;
 				}
-				if (getdatasize == maxvarsize) {
-					fwts_log_info(fw, "Variable is larger than maximum variable length.");
+
+				getvariable.Data = data;
+				ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
+				if (ioret == -1) {
+					fwts_log_info(fw, "Failed to get variable with variable larger than maximum variable length.");
 					fwts_uefi_print_status_info(fw, status);
 					free(data);
 					return FWTS_ERROR;
 				}
-				getdatasize += DATA_LENGTH;
-				getdatasize = (getdatasize > maxvarsize) ? maxvarsize : getdatasize;
-				data = realloc(data, getdatasize);
-				if (data) {
-					getvariable.DataSize = &getdatasize;
-					getvariable.Data = data;
-					continue;
-				} else {
-					fwts_log_info(fw, "Failed to allocate memory for test.");
-					return FWTS_ERROR;
-				}
 			}
-			break;
-		};
+		}
+
 		free(data);
 
 		(*usedvarssize) += getdatasize;