On 03/18/2013 08:40 AM, James Bottomley wrote:
> From: James Bottomley <JBottomley@xxxxxxxxxxxxx>
>
> The object here is to make the NV+BS variables accessible (at least read only)
> at runtime so we can get a full picture of the state of the EFI variables for
> debugging and secure boot purposes.
This should definitely be guarded with a debug config option and off by
default. It needs to be made really clear that this code is for
debugging purposes.
[...]
> +static efi_status_t setup_efi_bootvars(struct boot_params *params)
> +{
> + const int len = 1024;
> + efi_char16_t *str;
> + efi_status_t status;
> + struct setup_data **data = (struct setup_data **)¶ms->hdr.setup_data;
> +
> + status = efi_call_phys3(sys_table->boottime->allocate_pool,
> + EFI_LOADER_DATA, len, &str);
> + if (status != EFI_SUCCESS)
> + return status;
> +
> + /*
> + * Note: this trick only works on Little Endian if hdr.setup_data
> + * is u64 on both 64 and 32 bit
> + */
> + while (*data)
> + data = (struct setup_data **)&(*data)->next;
> +
> + memset(str, 0, len);
> +
> + for (;;) {
> + unsigned long size = len, str_len;
> + efi_guid_t guid;
> + struct efi_setup_bootvars *bvs;
> + unsigned int attributes;
> +
> + status = efi_call_phys3(sys_table->runtime->get_next_variable,
> + &size, str, &guid);
> + if (status != EFI_SUCCESS)
> + break;
> +
> + str_len = size;
Beware of these recent debacles,
http://article.gmane.org/gmane.linux.kernel.efi/905
http://article.gmane.org/gmane.linux.kernel.efi/909
I'm not saying you need to handle them explicitly in this patch, at
least not if this is all under a config option with a big fat warning.
> + size = 0;
> + status = efi_call_phys5(sys_table->runtime->get_variable,
> + str, &guid, &attributes, &size, NULL);
> + if (status != EFI_SUCCESS && status != EFI_BUFFER_TOO_SMALL)
> + break;
> +
> +
> + /*
> + * Please don't be tempted to optimise here: some
> + * UEFI implementations fail to set attributes if
> + * they return any error (including EFI_BUFFER_TOO_SMALL)
> + */
> + status = efi_call_phys3(sys_table->boottime->allocate_pool,
> + EFI_LOADER_DATA,
> + size + sizeof(*bvs) + str_len,
> + &bvs);
> + if (status != EFI_SUCCESS)
> + continue;
Allocating things as EFI_LOADER_DATA is fine but you need to be sure to
reserve these memory regions so that they don't get used by any of the
memory allocators. See
http://article.gmane.org/gmane.linux.kernel.efi/580
(yes, setup_efi_pci() suffers from the same problem)
> + memset(bvs, 0 , sizeof(bvs));
> + status = efi_call_phys5(sys_table->runtime->get_variable,
> + str, &guid, &attributes,
> + &size, bvs->data);
> +
> + if (status != EFI_SUCCESS
> + || (attributes & 0x07) != (EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE)) {
> + efi_call_phys1(sys_table->boottime->free_pool, bvs);
> + continue;
> + }
Please use the EFI_VARIABLE_* constants instead of the magic 0x07.
[...]
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index fff986d..931060a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -153,17 +153,103 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
> unsigned long *data_size,
> void *data)
> {
> - return efi_call_virt5(get_variable,
> - name, vendor, attr,
> - data_size, data);
> + efi_status_t status = efi_call_virt5(get_variable,
> + name, vendor, attr,
> + data_size, data);
> + u64 pa_data;
> + struct setup_data *s_d;
> +
> + if (status != EFI_NOT_FOUND)
> + return status;
> +
> +
> + for (pa_data = boot_params.hdr.setup_data; pa_data;
> + pa_data = s_d->next) {
> + struct efi_setup_bootvars *bvs;
> + efi_char16_t *bvs_name;
> + int i = 0;
> +
> + s_d = phys_to_virt(pa_data);
> +
> + if (s_d->type != SETUP_EFIVAR)
> + continue;
> +
> + bvs = (struct efi_setup_bootvars *)s_d;
> + bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
> +
> + for (i = 0; bvs_name[i] != 0 && name[i] != 0; i++)
> + if (bvs_name[i] != name[i])
> + break;
> +
> + if (bvs_name[i] != 0 || name[i] != 0)
> + continue;
> +
This would be cleaner if you pulled some of the utf16_str* functions out
of drivers/firmware/efivars.c and stuck them in include/linux/efi.h.
Then you could do something like,
if (utf16_strncmp(bvs_name, name, utf16_strlen(name)) != 0)
continue;
> + if (memcmp(&bvs->guid, vendor, sizeof(*vendor)) != 0)
> + continue;
> +
> + if (attr)
> + *attr = bvs->attributes;
> +
> + if (*data_size < bvs->size) {
> + *data_size = bvs->size;
> + return EFI_BUFFER_TOO_SMALL;
> + }
> + *data_size = bvs->size;
> + memcpy(data, bvs->data, bvs->size);
> +
> + return EFI_SUCCESS;
> + }
> + return EFI_NOT_FOUND;
> }
>
> static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
> efi_char16_t *name,
> efi_guid_t *vendor)
> {
> - return efi_call_virt3(get_next_variable,
> - name_size, name, vendor);
> + static int use_bootvars = 0;
> + efi_status_t status;
> + u64 pa_data = boot_params.hdr.setup_data;
> + int found = 0;
Why not make found a boolean?
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
