ACK: [PATCH 1/3] uefirtvariable: Check new VariableNameSize from GetNextVariableName()

[Colin Ian King]

On 05/03/13 21:54, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming at intel.com>
>
> Some firmware implementations update VariableNameSize in
> GetNextVariableName() with a value that is larger than the actual
> buffer required to hold the VariableName string. This is not
> technically a bug, but most implementations do update VariableNameSize
> with the value of strlen(VariableName) + 1, so print a warning if a
> different value is found.
>
> Signed-off-by: Matt Fleming <matt.fleming at intel.com>
> ---
>   src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
>   1 file changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index e00b343..177dbf9 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -226,7 +226,7 @@ static bool compare_name(uint16_t *name1, uint16_t *name2)
>   	return ident;
>   }
>
> -static int getnextvariable_test(fwts_framework *fw)
> +static int getnextvariable_test1(fwts_framework *fw)
>   {
>   	long ioret;
>   	uint64_t status;
> @@ -339,6 +339,72 @@ err_restore_env:
>   	return FWTS_ERROR;
>   }
>
> +/*
> + * Return true if variablenamesize is the length of the
> + * NULL-terminated unicode string, variablename.
> + */
> +static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize)
> +{
> +	uint64_t len;
> +	uint16_t c;
> +
> +	for (len = 2; len <= variablenamesize; len += sizeof(c)) {
> +		c = variablename[(len / sizeof(c)) - 1];
> +		if (!c)
> +			break;
> +	}
> +
> +	return len == variablenamesize;
> +}
> +
> +static int getnextvariable_test2(fwts_framework *fw)
> +{
> +	long ioret;
> +	uint64_t status;
> +
> +	struct efi_getnextvariablename getnextvariablename;
> +	uint64_t variablenamesize = MAX_DATA_LENGTH;
> +	uint16_t variablename[MAX_DATA_LENGTH];
> +	EFI_GUID vendorguid;
> +
> +	getnextvariablename.VariableNameSize = &variablenamesize;
> +	getnextvariablename.VariableName = variablename;
> +	getnextvariablename.VendorGuid = &vendorguid;
> +	getnextvariablename.status = &status;
> +
> +	/* To start the search, need to pass a Null-terminated string in VariableName */
> +	variablename[0] = '\0';
> +	while (true) {
> +		variablenamesize = MAX_DATA_LENGTH;
> +		ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename);
> +
> +		if (ioret == -1) {
> +
> +			/* no next variable was found*/
> +			if (*getnextvariablename.status == EFI_NOT_FOUND)
> +				break;
> +
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName",
> +				"Failed to get next variable name with UEFI runtime service.");
> +			fwts_uefi_print_status_info(fw, status);
> +			goto err;
> +		}
> +
> +		if (!strlen_valid(getnextvariablename.VariableName,
> +				  *getnextvariablename.VariableNameSize)) {
> +			fwts_warning(fw, "UEFIRuntimeGetNextVariableName "
> +				"Unexpected variable name size returned.");
> +			goto err;
> +		}
> +	};
> +
> +	return FWTS_OK;
> +
> +err:
> +
> +	return FWTS_ERROR;
> +}
> +
>   static int setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, uint64_t datasize,
>   					uint16_t *varname, EFI_GUID *gtestguid, uint8_t datadiff)
>   {
> @@ -857,7 +923,11 @@ static int uefirtvariable_test2(fwts_framework *fw)
>   {
>   	int ret;
>
> -	ret = getnextvariable_test(fw);
> +	ret = getnextvariable_test1(fw);
> +	if (ret != FWTS_OK)
> +		return ret;
> +
> +	ret = getnextvariable_test2(fw);
>   	if (ret != FWTS_OK)
>   		return ret;
>
>
Acked-by: Colin Ian King <colin.king at canonical.com>