On Tue, Mar 19, 2013 at 2:17 PM, Seiji Aguchi <[email protected]> wrote:
>
> [Problem]
> Some firmware implementations return the same variable name on multiple calls
> to
> get_next_variable().
> In this case, an efivar driver gets stuck in a infinite loop at initializing
> time,
> register_efivars().
> It is hard for users to fix the broken firmware.
>
> Here is an actual result of the case above.
> http://article.gmane.org/gmane.linux.kernel.efi/835
>
> [Solution]
> This patch terminates the loop immediately and disables get_next_variable()
> at the initializing time when the same variable name is found on multiple
> calls to get_next_variable().
>
> Also, to avoid stucking in the infinite loop,
> update_sysfs_entries returns without calling get_next_variable if the case
> above happens.
>
> Reported-by: Andre Heider <[email protected]>
> Reported-by: Lingzhu Xiang <[email protected]>
> Signed-off-by: Matt Fleming <[email protected]>
> Signed-off-by: Seiji Aguchi <[email protected]>
> ---
> drivers/firmware/efivars.c | 34 ++++++++++++++++++++++++++++++++--
> drivers/firmware/google/gsmi.c | 4 +++-
> include/linux/efi.h | 3 ++-
> 3 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index fe62aa3..fa601c6 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1725,6 +1725,14 @@ static void efivar_update_sysfs_entries(struct
> work_struct *work)
> efi_status_t status = EFI_NOT_FOUND;
> bool found;
>
> + spin_lock_irq(&efivars->lock);
> + if (!efivars->ops->get_next_variable) {
> + spin_unlock_irq(&efivars->lock);
> + pr_warn("efivars: Skip updating sysfs entries\n");
> + return;
> + }
> + spin_unlock_irq(&efivars->lock);
> +
> /* Add new sysfs entries */
> while (1) {
> variable_name = kzalloc(variable_name_size, GFP_KERNEL);
> @@ -1960,7 +1968,8 @@ EXPORT_SYMBOL_GPL(unregister_efivars);
>
> int register_efivars(struct efivars *efivars,
> const struct efivar_operations *ops,
> - struct kobject *parent_kobj)
> + struct kobject *parent_kobj,
> + bool *get_next_variable_cannot_call)
> {
> efi_status_t status = EFI_NOT_FOUND;
> efi_guid_t vendor_guid;
> @@ -2006,6 +2015,21 @@ int register_efivars(struct efivars *efivars,
> &vendor_guid);
> switch (status) {
> case EFI_SUCCESS:
> + /*
> + * Some firmware implementations return the
> + * same variable name on multiple calls to
> + * get_next_variable(). Terminate the loop
> + * immediately as there is no guarantee that
> + * we'll ever see a different variable name,
> + * and may end up looping here forever.
> + */
> + if (variable_is_present(variable_name, &vendor_guid))
> {
> + pr_warn("efivars: duplicate variable
> name.\n");
> + *get_next_variable_cannot_call = true;
> + status = EFI_NOT_FOUND;
> + break;
> + }
> +
> efivar_create_sysfs_entry(efivars,
> variable_name_size,
> variable_name,
> @@ -2056,6 +2080,7 @@ static int __init
> efivars_init(void)
> {
> int error = 0;
> + bool get_next_variable_cannot_call = false;
>
> printk(KERN_INFO "EFI Variables Facility v%s %s\n", EFIVARS_VERSION,
> EFIVARS_DATE);
> @@ -2075,7 +2100,12 @@ efivars_init(void)
> ops.get_next_variable = efi.get_next_variable;
> ops.query_variable_info = efi.query_variable_info;
>
> - error = register_efivars(&__efivars, &ops, efi_kobj);
> + error = register_efivars(&__efivars, &ops, efi_kobj,
> + &get_next_variable_cannot_call);
> + if (get_next_variable_cannot_call) {
> + pr_warn("efivars: Disable get_next_variable service\n");
> + ops.get_next_variable = NULL;
I don't like this pattern of clearing out ops fields. Can you instead
just add a flag in efivars that is checked before calling
ops->get_next_variable?
> + }
> if (error)
> goto err_put;
>
> diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
> index 91ddf0f..0c205c8 100644
> --- a/drivers/firmware/google/gsmi.c
> +++ b/drivers/firmware/google/gsmi.c
> @@ -778,6 +778,7 @@ static __init int gsmi_init(void)
> {
> unsigned long flags;
> int ret;
> + bool get_next_variable_cannot_call = false;
>
> ret = gsmi_system_valid();
> if (ret)
> @@ -893,7 +894,8 @@ static __init int gsmi_init(void)
> goto out_remove_bin_file;
> }
>
> - ret = register_efivars(&efivars, &efivar_ops, gsmi_kobj);
> + ret = register_efivars(&efivars, &efivar_ops, gsmi_kobj,
> + &get_next_variable_cannot_call);
> if (ret) {
> printk(KERN_INFO "gsmi: Failed to register efivars\n");
> goto out_remove_sysfs_files;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 9bf2f1f..9db51b1 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -756,7 +756,8 @@ struct efivars {
>
> int register_efivars(struct efivars *efivars,
> const struct efivar_operations *ops,
> - struct kobject *parent_kobj);
> + struct kobject *parent_kobj,
> + bool *get_next_variable_cannot_call);
> void unregister_efivars(struct efivars *efivars);
>
> #endif /* CONFIG_EFI_VARS */
> -- 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
