[PATCH 1/2] uefi: uefirtvariable: Add invalid NULL parameter sanity checks
Colin Ian King
colin.king at canonical.com
Wed Apr 8 08:51:53 UTC 2015
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 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 | 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 = >estguid1;
>> + 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 = >estguid1;
>> + 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 = >estguid1;
>> + getvariable.Attributes = &attr;
>> + getvariable.DataSize = NULL;
>> + getvariable.Data = data;
>> + getvariable.status = &status;
>> + getvariable_test_invalid(fw, &getvariable, "NULL datasize");
>> +
>> + getvariable.VariableName = variablenametest;
>> + getvariable.VendorGuid = >estguid1;
>> + 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 }
>> };
>>
>
More information about the fwts-devel
mailing list