[PATCH 5/5] uefirtvariable: allow large sizes for variable names
Colin Ian King
colin.king at canonical.com
Tue Feb 10 14:13:15 UTC 2015
On 06/02/15 03:50, Ricardo Neri wrote:
> The UEFI specification does not define the maximum length for the
> variable name. Thus, it may happen that some firmware implementations
> have variable names longer than 1024 characters. Rather than limiting
> the maximum size to 1024 characters, set the initial size to 1024 chars
> and enlarge as required.
>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon at linux.intel.com>
> ---
> efi_runtime/efi_runtime.c | 3 -
> src/uefi/uefirtvariable/uefirtvariable.c | 109 +++++++++++++++++++++++++++----
> 2 files changed, 96 insertions(+), 16 deletions(-)
>
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index 786a1df..1125556 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -450,9 +450,6 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
> sizeof(vendor_guid)))
> return -EFAULT;
>
> - if (name_size > 1024)
> - return -EFAULT;
> -
> convert_from_guid(&vendor, &vendor_guid);
>
> rv = copy_ucs2_from_user_len(&name,
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index 0c3f532..056a5cb 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -295,7 +295,8 @@ static int getnextvariable_test1(fwts_framework *fw)
>
> struct efi_getnextvariablename getnextvariablename;
> uint64_t variablenamesize = MAX_DATA_LENGTH;
> - uint16_t variablename[MAX_DATA_LENGTH];
> + uint64_t maxvariablenamesize = variablenamesize;
> + uint16_t *variablename;
> EFI_GUID vendorguid;
> bool found_name = false, found_guid = false;
>
> @@ -329,6 +330,11 @@ static int getnextvariable_test1(fwts_framework *fw)
> return FWTS_ERROR;
> }
>
> + variablename = malloc(sizeof(uint16_t) * variablenamesize);
> + if (!variablename) {
> + fwts_skipped(fw, "Unable to alloc memory for variable name");
> + return FWTS_SKIP;
> + }
> getnextvariablename.VariableNameSize = &variablenamesize;
> getnextvariablename.VariableName = variablename;
> getnextvariablename.VendorGuid = &vendorguid;
> @@ -340,7 +346,7 @@ static int getnextvariable_test1(fwts_framework *fw)
> */
> variablename[0] = '\0';
> while (true) {
> - variablenamesize = MAX_DATA_LENGTH;
> + variablenamesize = maxvariablenamesize;
> ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename);
>
> if (ioret == -1) {
> @@ -349,6 +355,20 @@ static int getnextvariable_test1(fwts_framework *fw)
> if (*getnextvariablename.status == EFI_NOT_FOUND)
> break;
>
> + /*
> + * If the buffer we provided is too small for the name,
> + * allocate a larger buffer and try again
> + */
> + if (*getnextvariablename.status == EFI_BUFFER_TOO_SMALL) {
> + variablename = realloc(variablename,
> + sizeof(uint16_t) * variablenamesize);
If realloc fails, the original allocation is left untouched and not
freed (or moved) and variablename is assigned NULL. This will lead to a
small memory leak. Perhaps:
uint16_t *tmp;
tmp = realloc(variablename,
sizeof(uint16_t) * variablenamesize);
if (!tmp) {
fwts_skipped(fw, "Unable to reallocate memory for variable name");
free(variablename);
return FWTS_SKIP;
}
> + if (variablename) {
> + getnextvariablename.VariableName = variablename;
> + maxvariablenamesize = variablenamesize;
> + continue;
> + }
> + }
> +
> fwts_failed(fw, LOG_LEVEL_HIGH,
> "UEFIRuntimeGetNextVariableName",
> "Failed to get next variable name with UEFI "
> @@ -364,6 +384,12 @@ static int getnextvariable_test1(fwts_framework *fw)
> break;
> };
>
> + if (variablename) {
> + free(variablename);
> + getnextvariablename.VariableName = NULL;
> + variablename = NULL;
> + }
> +
> if (!found_name) {
> fwts_failed(fw, LOG_LEVEL_HIGH,
> "UEFIRuntimeGetNextVariableNameName",
> @@ -404,6 +430,9 @@ err_restore_env:
> return FWTS_ERROR;
> }
>
> + if (variablename)
> + free(variablename);
> +
> return FWTS_ERROR;
> }
>
> @@ -434,9 +463,16 @@ static int getnextvariable_test2(fwts_framework *fw)
>
> struct efi_getnextvariablename getnextvariablename;
> uint64_t variablenamesize = MAX_DATA_LENGTH;
> - uint16_t variablename[MAX_DATA_LENGTH];
> + uint64_t maxvariablenamesize = variablenamesize;
> + uint16_t *variablename;
> EFI_GUID vendorguid;
>
> + variablename = malloc(sizeof(uint16_t) * variablenamesize);
> + if (!variablename) {
> + fwts_skipped(fw, "Unable to alloc memory for variable name");
> + return FWTS_SKIP;
> + }
> +
> getnextvariablename.VariableNameSize = &variablenamesize;
> getnextvariablename.VariableName = variablename;
> getnextvariablename.VendorGuid = &vendorguid;
> @@ -448,7 +484,7 @@ static int getnextvariable_test2(fwts_framework *fw)
> */
> variablename[0] = '\0';
> while (true) {
> - variablenamesize = MAX_DATA_LENGTH;
> + variablenamesize = maxvariablenamesize;
> ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename);
>
> if (ioret == -1) {
> @@ -456,6 +492,19 @@ static int getnextvariable_test2(fwts_framework *fw)
> /* no next variable was found*/
> if (*getnextvariablename.status == EFI_NOT_FOUND)
> break;
> + /*
> + * If the buffer we provided is too small for the name,
> + * allocate a larger buffer and try again
> + */
> + if (*getnextvariablename.status == EFI_BUFFER_TOO_SMALL) {
> + variablename = realloc(variablename,
> + sizeof(uint16_t) * variablenamesize);
same as mentioned above with realloc.
> + if (variablename) {
> + getnextvariablename.VariableName = variablename;
> + maxvariablenamesize = variablenamesize;
> + continue;
> + }
> + }
>
> fwts_failed(fw, LOG_LEVEL_HIGH,
> "UEFIRuntimeGetNextVariableName",
> @@ -465,17 +514,20 @@ static int getnextvariable_test2(fwts_framework *fw)
> goto err;
> }
>
> - if (variablenamesize != MAX_DATA_LENGTH &&
> - !strlen_valid(variablename, variablenamesize)) {
> + if (!strlen_valid(variablename, variablenamesize)) {
> fwts_warning(fw, "UEFIRuntimeGetNextVariableName "
> "Unexpected variable name size returned.");
> goto err;
> }
> };
>
> + if (variablename)
> + free(variablename);
> return FWTS_OK;
>
> err:
> + if (variablename)
> + free(variablename);
> return FWTS_ERROR;
> }
>
> @@ -564,9 +616,16 @@ static int getnextvariable_test3(fwts_framework *fw)
>
> struct efi_getnextvariablename getnextvariablename;
> uint64_t variablenamesize = MAX_DATA_LENGTH;
> - uint16_t variablename[MAX_DATA_LENGTH];
> + uint64_t maxvariablenamesize = variablenamesize;
> + uint16_t *variablename;
> EFI_GUID vendorguid;
> - char name[MAX_DATA_LENGTH];
> + char *name;
> +
> + variablename = malloc(sizeof(uint16_t) * variablenamesize);
> + if (!variablename) {
> + fwts_skipped(fw, "Unable to alloc memory for variable name");
> + return FWTS_SKIP;
> + }
>
> getnextvariablename.VariableNameSize = &variablenamesize;
> getnextvariablename.VariableName = variablename;
> @@ -581,7 +640,7 @@ static int getnextvariable_test3(fwts_framework *fw)
> while (true) {
> struct efi_var_item *item;
>
> - variablenamesize = MAX_DATA_LENGTH;
> + variablenamesize = maxvariablenamesize;
> ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename);
>
> if (ioret == -1) {
> @@ -589,6 +648,19 @@ static int getnextvariable_test3(fwts_framework *fw)
> /* no next variable was found*/
> if (*getnextvariablename.status == EFI_NOT_FOUND)
> break;
> + /*
> + * if the buffer we provided is too small for the name,
> + * allocate a larger buffer and try again
> + */
> + if (*getnextvariablename.status == EFI_BUFFER_TOO_SMALL) {
> + variablename = realloc(variablename,
> + sizeof(uint16_t) * variablenamesize);
and here too
> + if (variablename) {
> + getnextvariablename.VariableName = variablename;
> + maxvariablenamesize = variablenamesize;
> + continue;
> + }
> + }
>
> fwts_failed(fw, LOG_LEVEL_HIGH,
> "UEFIRuntimeGetNextVariableName",
> @@ -634,10 +706,17 @@ static int getnextvariable_test3(fwts_framework *fw)
> item->hash = hash_func(variablename, variablenamesize);
>
> if (bucket_insert(item)) {
> - fwts_uefi_str16_to_str(name, sizeof(name), variablename);
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "UEFIRuntimeGetNextVariableName",
> - "Duplicate variable name %s found.", name);
> + name = malloc(variablenamesize * sizeof(char));
> + if (name) {
> + fwts_uefi_str16_to_str(name, sizeof(name), variablename);
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "UEFIRuntimeGetNextVariableName",
> + "Duplicate variable name %s found.", name);
> + free(name);
> + } else
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "UEFIRuntimeGetNextVariableName",
> + "Duplicate variable name found (too long name).");
> free(item->name);
> free(item->guid);
> free(item);
> @@ -646,10 +725,14 @@ static int getnextvariable_test3(fwts_framework *fw)
> };
>
> bucket_destroy();
> + if (variablename)
> + free(variablename);
> return FWTS_OK;
>
> err:
> bucket_destroy();
> + if (variablename)
> + free(variablename);
> return FWTS_ERROR;
> }
>
>
More information about the fwts-devel
mailing list