ACK: [PATCH 1/2][V2] uefi: uefirtvariable: Add invalid NULL parameter sanity checks

Alex Hung alex.hung at canonical.com
Wed Apr 15 02:12:31 UTC 2015 


On 15-04-08 06:58 PM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> UEFI allows NULL parameters to be passed in the GetVariable
> run time service, so add NULL parameter checks to this test.
>
> We need to modify the uefi driver to handle NULL parameters too.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>   efi_runtime/efi_runtime.c                |  60 +++++++++------
>   src/uefi/uefirtvariable/uefirtvariable.c | 123 +++++++++++++++++++++++++++++++
>   2 files changed, 160 insertions(+), 23 deletions(-)
>
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index 31e5bb3..5fe1084 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -237,13 +237,12 @@ static long efi_runtime_get_variable(unsigned long arg)
>   {
>   	struct efi_getvariable __user *pgetvariable;
>   	struct efi_getvariable pgetvariable_local;
> -	unsigned long datasize, prev_datasize;
> -	EFI_GUID vendor_guid;
> -	efi_guid_t vendor;
> +	unsigned long datasize, prev_datasize, *pdatasize;
> +	efi_guid_t vendor, *pvendor = NULL;
>   	efi_status_t status;
> -	uint16_t *name;
> -	uint32_t attr;
> -	void *data;
> +	uint16_t *name = NULL;
> +	uint32_t attr, *pattr;
> +	void *data = NULL;
>   	int rv = 0;
>   
>   	pgetvariable = (struct efi_getvariable __user *)arg;
> @@ -251,31 +250,45 @@ static long efi_runtime_get_variable(unsigned long arg)
>   	if (copy_from_user(&pgetvariable_local, pgetvariable,
>   			   sizeof(pgetvariable_local)))
>   		return -EFAULT;
> +	if (pgetvariable_local.DataSize &&
> +	    get_user(datasize, pgetvariable_local.DataSize))
> +		return -EFAULT;
> +	if (pgetvariable_local.VendorGuid) {
> +		EFI_GUID vendor_guid;
>   
> -	if (get_user(datasize, pgetvariable_local.DataSize) ||
> -	    copy_from_user(&vendor_guid, pgetvariable_local.VendorGuid,
> +		if (copy_from_user(&vendor_guid, pgetvariable_local.VendorGuid,
>   			   sizeof(vendor_guid)))
> -		return -EFAULT;
> +			return -EFAULT;
> +		convert_from_guid(&vendor, &vendor_guid);
> +		pvendor = &vendor;
> +	}
>   
> -	convert_from_guid(&vendor, &vendor_guid);
> +	if (pgetvariable_local.VariableName) {
> +		rv = copy_ucs2_from_user(&name, pgetvariable_local.VariableName);
> +		if (rv)
> +			return rv;
> +	}
>   
> -	rv = copy_ucs2_from_user(&name, pgetvariable_local.VariableName);
> -	if (rv)
> -		return rv;
> +	pattr = pgetvariable_local.Attributes ? &attr : NULL;
> +	pdatasize = pgetvariable_local.DataSize ? &datasize : NULL;
>   
> -	data = kmalloc(datasize, GFP_KERNEL);
> -	if (!data) {
> -		ucs2_kfree(name);
> -		return -ENOMEM;
> +	if (pgetvariable_local.DataSize && pgetvariable_local.Data) {
> +		data = kmalloc(datasize, GFP_KERNEL);
> +		if (!data) {
> +			ucs2_kfree(name);
> +			return -ENOMEM;
> +		}
>   	}
>   
>   	prev_datasize = datasize;
> -	status = efi.get_variable(name, &vendor, &attr, &datasize, data);
> +	status = efi.get_variable(name, pvendor, pattr, pdatasize, data);
>   	ucs2_kfree(name);
>   
> -	if (status == EFI_SUCCESS && prev_datasize >= datasize)
> -		rv = copy_to_user(pgetvariable_local.Data, data, datasize);
> -	kfree(data);
> +	if (data) {
> +		if (status == EFI_SUCCESS && prev_datasize >= datasize)
> +			rv = copy_to_user(pgetvariable_local.Data, data, datasize);
> +		kfree(data);
> +	}
>   
>   	if (rv)
>   		return rv;
> @@ -283,8 +296,9 @@ static long efi_runtime_get_variable(unsigned long arg)
>   	if (put_user(status, pgetvariable_local.status))
>   		return -EFAULT;
>   	if (status == EFI_SUCCESS && prev_datasize >= datasize) {
> -		if (put_user(attr, pgetvariable_local.Attributes) ||
> -		    put_user(datasize, pgetvariable_local.DataSize))
> +		if (pattr && put_user(attr, pgetvariable_local.Attributes))
> +			return -EFAULT;
> +		if (pdatasize && put_user(datasize, pgetvariable_local.DataSize))
>   			return -EFAULT;
>   		return 0;
>   	} else {
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index ddf3885..0617ff4 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -1773,6 +1773,128 @@ static int uefirtvariable_test7(fwts_framework *fw)
>   	return FWTS_OK;
>   }
>   
> +static void getvariable_test_invalid(
> +	fwts_framework *fw,
> +	struct efi_getvariable *getvariable,
> +	const char *test)
> +{
> +	long ioret;
> +
> +	fwts_log_info(fw, "Testing GetVariable with %s.", test);
> +
> +	ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, getvariable);
> +	if (ioret == -1) {
> +		if (*(getvariable->status) == EFI_INVALID_PARAMETER) {
> +			fwts_passed(fw, "GetVariable with %s returned error "
> +				"EFI_INVALID_PARAMETER as expected.", test);
> +			return;
> +		}
> +		fwts_failed(fw, LOG_LEVEL_HIGH,
> +			"UEFIRuntimeGetVariableInvalid",
> +			"GetVariable with %s failed to get expected error return "
> +			"status, expected EFI_INVALID_PARAMETER.", test);
> +		fwts_uefi_print_status_info(fw, *(getvariable->status));
> +		return;
> +	}
> +	fwts_failed(fw, LOG_LEVEL_HIGH,
> +		"UEFIRuntimeGetVariableInvalid",
> +		"GetVariable wuth %s failed to get an error return status, "
> +		"expected EFI_INVALID_PARAMETER.", test);
> +
> +	return;
> +
> +}
> +
> +static int uefirtvariable_test8(fwts_framework *fw)
> +{
> +	struct efi_getvariable getvariable;
> +	struct efi_setvariable setvariable;
> +	uint8_t data[16];
> +	uint64_t status, dataindex;
> +	uint64_t getdatasize = sizeof(data);
> +	uint32_t attr;
> +	int ioret;
> +
> +	for (dataindex = 0; dataindex < sizeof(data); dataindex++)
> +		data[dataindex] = (uint8_t)dataindex;
> +
> +	setvariable.VariableName = variablenametest;
> +	setvariable.VendorGuid = &gtestguid1;
> +	setvariable.Attributes = attributes;
> +	setvariable.DataSize = sizeof(data);
> +	setvariable.Data = data;
> +	setvariable.status = &status;
> +
> +	ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
> +	if (ioret == -1) {
> +		if (status == EFI_OUT_OF_RESOURCES) {
> +			fwts_uefi_print_status_info(fw, status);
> +			fwts_skipped(fw,
> +				"Run out of resources for SetVariable UEFI "
> +				"runtime interface: cannot test.");
> +			fwts_advice(fw,
> +				"Firmware may reclaim some resources after "
> +				"rebooting. Reboot and test again may be "
> +				"helpful to continue the test.");
> +			return FWTS_SKIP;
> +		}
> +		fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
> +			"Failed to set variable with UEFI runtime service.");
> +		return FWTS_ERROR;
> +	}
> +
> +	getvariable.VariableName = NULL;
> +	getvariable.VendorGuid = &gtestguid1;
> +	getvariable.Attributes = &attr;
> +	getvariable.DataSize = &getdatasize;
> +	getvariable.Data = data;
> +	getvariable.status = &status;
> +	getvariable_test_invalid(fw, &getvariable, "NULL variable name");
> +
> +	getvariable.VariableName = variablenametest;
> +	getvariable.VendorGuid = NULL;
> +	getvariable.Attributes = &attr;
> +	getvariable.DataSize = &getdatasize;
> +	getvariable.Data = data;
> +	getvariable.status = &status;
> +	getvariable_test_invalid(fw, &getvariable, "NULL vendor GUID");
> +
> +	getvariable.VariableName = variablenametest;
> +	getvariable.VendorGuid = &gtestguid1;
> +	getvariable.Attributes = &attr;
> +	getvariable.DataSize = NULL;
> +	getvariable.Data = data;
> +	getvariable.status = &status;
> +	getvariable_test_invalid(fw, &getvariable, "NULL datasize");
> +
> +	getvariable.VariableName = variablenametest;
> +	getvariable.VendorGuid = &gtestguid1;
> +	getvariable.Attributes = &attr;
> +	getvariable.DataSize = &getdatasize;
> +	getvariable.Data = NULL;
> +	getvariable.status = &status;
> +	getvariable_test_invalid(fw, &getvariable, "NULL data");
> +
> +	getvariable.VariableName = NULL;
> +	getvariable.VendorGuid = NULL;
> +	getvariable.Attributes = &attr;
> +	getvariable.DataSize = NULL;
> +	getvariable.Data = NULL;
> +	getvariable.status = &status;
> +	getvariable_test_invalid(fw, &getvariable, "NULL variable name, vendor GUID, datasize and data");
> +
> +	/* delete the variable */
> +	setvariable.DataSize = 0;
> +	ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
> +	if (ioret == -1) {
> +		fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
> +			"Failed to delete variable with UEFI runtime service.");
> +		fwts_uefi_print_status_info(fw, status);
> +		return FWTS_ERROR;
> +	}
> +	return FWTS_OK;
> +}
> +
>   static int options_check(fwts_framework *fw)
>   {
>   	FWTS_UNUSED(fw);
> @@ -1843,6 +1965,7 @@ static fwts_framework_minor_test uefirtvariable_tests[] = {
>   	{ uefirtvariable_test5, "Test UEFI RT service variable interface stress test." },
>   	{ uefirtvariable_test6, "Test UEFI RT service set variable interface stress test." },
>   	{ uefirtvariable_test7, "Test UEFI RT service query variable info interface stress test." },
> +	{ uefirtvariable_test8, "Test UEFI RT service get variable interface, invalid parameters." },
>   	{ NULL, NULL }
>   };
>   

Acked-by: Alex Hung <alex.hung at canonical.com>

More information about the fwts-devel mailing list