diff mbox

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

Message ID 1427990400-3374-2-git-send-email-colin.king@canonical.com
State Rejected
Headers show

Commit Message

Colin Ian King April 2, 2015, 3:59 p.m. UTC 
From: Colin Ian King <colin.king@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@canonical.com>
---
 efi_runtime/efi_runtime.c                |  59 +++++++++------
 src/uefi/uefirtvariable/uefirtvariable.c | 123 +++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+), 23 deletions(-)

Comments

Alex Hung April 7, 2015, 11:56 p.m. UTC | #1 
On 15-04-02 11:59 PM, Colin King wrote:
> From: Colin Ian King <colin.king@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@canonical.com>
> ---
>   efi_runtime/efi_runtime.c                |  59 +++++++++------
>   src/uefi/uefirtvariable/uefirtvariable.c | 123 +++++++++++++++++++++++++++++++
>   2 files changed, 159 insertions(+), 23 deletions(-)
>
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index 31e5bb3..86cda1a 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,44 @@ static long efi_runtime_get_variable(unsigned long arg)
>   	if (copy_from_user(&pgetvariable_local, pgetvariable,
>   			   sizeof(pgetvariable_local)))
>   		return -EFAULT;
> +	if (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.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 +295,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@canonical.com>
Heyi Guo April 8, 2015, 3:28 a.m. UTC | #2 
Hi Colin,

I think DataSize may also be a null pointer, and it is better to add 
sanity check for it too.

pgetvariable_local.DataSize


On 04/02/2015 11:59 PM, Colin King wrote:
> From: Colin Ian King <colin.king@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@canonical.com>
> ---
>   efi_runtime/efi_runtime.c                |  59 +++++++++------
>   src/uefi/uefirtvariable/uefirtvariable.c | 123 +++++++++++++++++++++++++++++++
>   2 files changed, 159 insertions(+), 23 deletions(-)
>
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index 31e5bb3..86cda1a 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,44 @@ static long efi_runtime_get_variable(unsigned long arg)
>   	if (copy_from_user(&pgetvariable_local, pgetvariable,
>   			   sizeof(pgetvariable_local)))
>   		return -EFAULT;
> +	if (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.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 +295,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 }
>   };
>
Colin Ian King April 8, 2015, 8:51 a.m. UTC | #3 
On 08/04/15 04:28, Heyi Guo wrote:
> Hi Colin,
> 
> I think DataSize may also be a null pointer, and it is better to add
> sanity check for it too.

Isn't that already in the patch? See comments below..

> 
> pgetvariable_local.DataSize
> 
> 
> On 04/02/2015 11:59 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@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@canonical.com>
>> ---
>>   efi_runtime/efi_runtime.c                |  59 +++++++++------
>>   src/uefi/uefirtvariable/uefirtvariable.c | 123
>> +++++++++++++++++++++++++++++++
>>   2 files changed, 159 insertions(+), 23 deletions(-)
>>
>> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
>> index 31e5bb3..86cda1a 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,44 @@ static long efi_runtime_get_variable(unsigned
>> long arg)
>>       if (copy_from_user(&pgetvariable_local, pgetvariable,
>>                  sizeof(pgetvariable_local)))
>>           return -EFAULT;
>> +    if (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;

NULL DataSize -> pdatasize

>>   -    data = kmalloc(datasize, GFP_KERNEL);
>> -    if (!data) {
>> -        ucs2_kfree(name);
>> -        return -ENOMEM;
>> +    if (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);

pdatasize being used here, which can be NULL

>>       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 +295,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;

Null check for DataSize

>> +    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 }
>>   };
>>   
>
Heyi Guo April 8, 2015, 9:04 a.m. UTC | #4 
Sorry, missed that.

However, why not to add the check before get_user?

+    if (get_user(datasize, pgetvariable_local.DataSize))
+        return -EFAULT;



On 04/08/2015 04:51 PM, Colin Ian King wrote:
> +    if (get_user(datasize, pgetvariable_local.DataSize))
> >>+        return -EFAULT;
Colin Ian King April 8, 2015, 9:09 a.m. UTC | #5 
On 08/04/15 10:04, Heyi Guo wrote:
> Sorry, missed that.
> 
> However, why not to add the check before get_user?
> 
> +    if (get_user(datasize, pgetvariable_local.DataSize))
> +        return -EFAULT;
> 

Not sure what you mean by that.

> 
> 
> On 04/08/2015 04:51 PM, Colin Ian King wrote:
>> +    if (get_user(datasize, pgetvariable_local.DataSize))
>> >>+        return -EFAULT;
>
Heyi Guo April 8, 2015, 9:31 a.m. UTC | #6 
Yes there is check for DataSize; sorry I missed the code at the 1st time.

However, in below code, pgetvariable_local.DataSize will be dereferenced 
and there is no check before it. Not sure if it is an issue.

+    if (get_user(datasize, pgetvariable_local.DataSize))


On 04/08/2015 04:51 PM, Colin Ian King wrote:
> On 08/04/15 04:28, Heyi Guo wrote:
>> Hi Colin,
>>
>> I think DataSize may also be a null pointer, and it is better to add
>> sanity check for it too.
> Isn't that already in the patch? See comments below..
>
>> pgetvariable_local.DataSize
>>
>>
>> On 04/02/2015 11:59 PM, Colin King wrote:
>>> From: Colin Ian King <colin.king@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@canonical.com>
>>> ---
>>>    efi_runtime/efi_runtime.c                |  59 +++++++++------
>>>    src/uefi/uefirtvariable/uefirtvariable.c | 123
>>> +++++++++++++++++++++++++++++++
>>>    2 files changed, 159 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
>>> index 31e5bb3..86cda1a 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,44 @@ static long efi_runtime_get_variable(unsigned
>>> long arg)
>>>        if (copy_from_user(&pgetvariable_local, pgetvariable,
>>>                   sizeof(pgetvariable_local)))
>>>            return -EFAULT;
>>> +    if (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;
> NULL DataSize -> pdatasize
>
>>>    -    data = kmalloc(datasize, GFP_KERNEL);
>>> -    if (!data) {
>>> -        ucs2_kfree(name);
>>> -        return -ENOMEM;
>>> +    if (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);
> pdatasize being used here, which can be NULL
>
>>>        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 +295,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;
> Null check for DataSize
>
>>> +    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 }
>>>    };
>>>
diff mbox

Patch

diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
index 31e5bb3..86cda1a 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,44 @@  static long efi_runtime_get_variable(unsigned long arg)
 	if (copy_from_user(&pgetvariable_local, pgetvariable,
 			   sizeof(pgetvariable_local)))
 		return -EFAULT;
+	if (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.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 +295,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 }
 };