[PATCH 1/2] uefi: uefirtvariable: Add invalid NULL parameter sanity checks
Heyi Guo
heyi.guo at linaro.org
Wed Apr 8 09:31:33 UTC 2015
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 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