efi: vars: Don't drop lock in the middle of efivar_init()

JIRA: https://issues.redhat.com/browse/RHEL-2564

commit ec3507b2ca51286de6ecd85fdac8e722219cdef8
Author: Ard Biesheuvel <ardb@kernel.org>
Date:   Mon Jun 20 17:04:32 2022 +0200

    efi: vars: Don't drop lock in the middle of efivar_init()

    Even though the efivars_lock lock is documented as protecting the
    efivars->ops pointer (among other things), efivar_init() happily
    releases and reacquires the lock for every EFI variable that it
    enumerates. This used to be needed because the lock was originally a
    spinlock, which prevented the callback that is invoked for every
    variable from being able to sleep. However, releasing the lock could
    potentially invalidate the ops pointer, but more importantly, it might
    allow a SetVariable() runtime service call to take place concurrently,
    and the UEFI spec does not define how this affects an enumeration that
    is running in parallel using the GetNextVariable() runtime service,
    which is what efivar_init() uses.

    In the meantime, the lock has been converted into a semaphore, and the
    only reason we need to drop the lock is because the efivarfs pseudo
    filesystem driver will otherwise deadlock when it invokes the efivars
    API from the callback to create the efivar_entry items and insert them
    into the linked list. (EFI pstore is affected in a similar way)

    So let's switch to helpers that can be used while the lock is already
    taken. This way, we can hold on to the lock throughout the enumeration.

    Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>

drivers/firmware/efi/efi-pstore.c

@@ -364,7 +364,6 @@ static int efi_pstore_callback(efi_char16_t *name, efi_guid_t vendor,
 			       unsigned long name_size, void *data)
 {
 	struct efivar_entry *entry;
-	int ret;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry)
@@ -373,11 +372,9 @@ static int efi_pstore_callback(efi_char16_t *name, efi_guid_t vendor,
 	memcpy(entry->var.VariableName, name, name_size);
 	entry->var.VendorGuid = vendor;
 
-	ret = efivar_entry_add(entry, &efi_pstore_list);
-	if (ret)
-		kfree(entry);
+	__efivar_entry_add(entry, &efi_pstore_list);
 
-	return ret;
+	return 0;
 }
 
 static int efi_pstore_update_entry(efi_char16_t *name, efi_guid_t vendor,

drivers/firmware/efi/efivars.c

@@ -527,10 +527,7 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
 	}
 
 	kobject_uevent(&new_var->kobj, KOBJ_ADD);
-	if (efivar_entry_add(new_var, &efivar_sysfs_list)) {
-		efivar_unregister(new_var);
-		return -EINTR;
-	}
+	__efivar_entry_add(new_var, &efivar_sysfs_list);
 
 	return 0;
 }

drivers/firmware/efi/vars.c

@@ -450,9 +450,6 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 						&vendor_guid);
 		switch (status) {
 		case EFI_SUCCESS:
-			if (duplicates)
-				up(&efivars_lock);
-
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);
 
@@ -476,14 +473,6 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 				if (err)
 					status = EFI_NOT_FOUND;
 			}
-
-			if (duplicates) {
-				if (down_interruptible(&efivars_lock)) {
-					err = -EINTR;
-					goto free;
-				}
-			}
-
 			break;
 		case EFI_UNSUPPORTED:
 			err = -EOPNOTSUPP;
@@ -526,6 +515,17 @@ int efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
 }
 EXPORT_SYMBOL_GPL(efivar_entry_add);
 
+/**
+ * __efivar_entry_add - add entry to variable list
+ * @entry: entry to add to list
+ * @head: list head
+ */
+void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
+{
+	list_add(&entry->list, head);
+}
+EXPORT_SYMBOL_GPL(__efivar_entry_add);
+
 /**
  * efivar_entry_remove - remove entry from variable list
  * @entry: entry to remove from list

fs/efivarfs/super.c

@@ -199,10 +199,8 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 		goto fail_inode;
 	}
 
-	efivar_entry_size(entry, &size);
-	err = efivar_entry_add(entry, &efivarfs_list);
-	if (err)
-		goto fail_inode;
+	__efivar_entry_get(entry, NULL, &size, NULL);
+	__efivar_entry_add(entry, &efivarfs_list);
 
 	/* copied by the above to local storage in the dentry. */
 	kfree(name);

include/linux/efi.h

@@ -1126,6 +1126,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		void *data, bool duplicates, struct list_head *head);
 
 int efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
+void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
 int efivar_entry_remove(struct efivar_entry *entry);
 
 int __efivar_entry_delete(struct efivar_entry *entry);