From 8ca869b24538a7b5501af368e87e4a59b0c04117 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Wed, 22 Jun 2022 09:31:31 +0200 Subject: pstore: Add priv field to pstore_record for backend specific use The EFI pstore backend will need to store per-record variable name data when we switch away from the efivars layer. Add a priv field to struct pstore_record, and document it as holding a backend specific pointer that is assumed to be a kmalloc()d buffer, and will be kfree()d when the entire record is freed. Acked-by: Kees Cook Signed-off-by: Ard Biesheuvel --- fs/pstore/inode.c | 1 + fs/pstore/platform.c | 1 + 2 files changed, 2 insertions(+) (limited to 'fs') diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c index 14658b009f1b..ffbadb8b3032 100644 --- a/fs/pstore/inode.c +++ b/fs/pstore/inode.c @@ -55,6 +55,7 @@ static void free_pstore_private(struct pstore_private *private) return; if (private->record) { kfree(private->record->buf); + kfree(private->record->priv); kfree(private->record); } kfree(private); diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index e26162f102ff..0c034ea39954 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -769,6 +769,7 @@ void pstore_get_backend_records(struct pstore_info *psi, if (rc) { /* pstore_mkfile() did not take record, so free it. */ kfree(record->buf); + kfree(record->priv); kfree(record); if (rc != -EEXIST || !quiet) failed++; -- cgit v1.2.3 From ec3507b2ca51286de6ecd85fdac8e722219cdef8 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Mon, 20 Jun 2022 17:04:32 +0200 Subject: 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 --- drivers/firmware/efi/efi-pstore.c | 7 ++----- drivers/firmware/efi/efivars.c | 5 +---- drivers/firmware/efi/vars.c | 22 +++++++++++----------- fs/efivarfs/super.c | 6 ++---- include/linux/efi.h | 1 + 5 files changed, 17 insertions(+), 24 deletions(-) (limited to 'fs') diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index 7e771c56c13c..0d80cc7ff6ca 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/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, diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c index ea0bc39dc965..c19db0b35c0d 100644 --- a/drivers/firmware/efi/efivars.c +++ b/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; } diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index cae590bd08f2..146360e2f1cb 100644 --- a/drivers/firmware/efi/vars.c +++ b/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 diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index 15880a68faad..09dfa8362f50 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -155,10 +155,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); diff --git a/include/linux/efi.h b/include/linux/efi.h index 53f64c14a525..56f04b6daeb0 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1064,6 +1064,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); -- cgit v1.2.3 From 3a75f9f2f9ad19bb9a0f566373ae91d8f09db85e Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Tue, 21 Jun 2022 15:48:29 +0200 Subject: efi: vars: Use locking version to iterate over efivars linked lists Both efivars and efivarfs uses __efivar_entry_iter() to go over the linked list that shadows the list of EFI variables held by the firmware, but fail to call the begin/end helpers that are documented as a prerequisite. So switch to the proper version, which is efivar_entry_iter(). Given that in both cases, efivar_entry_remove() is invoked with the lock held already, don't take the lock there anymore. Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/efivars.c | 8 ++------ drivers/firmware/efi/vars.c | 9 +-------- fs/efivarfs/super.c | 9 +++------ include/linux/efi.h | 2 +- 4 files changed, 7 insertions(+), 21 deletions(-) (limited to 'fs') diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c index 8341fb15f62e..801a65582172 100644 --- a/drivers/firmware/efi/efivars.c +++ b/drivers/firmware/efi/efivars.c @@ -602,10 +602,7 @@ static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor, static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data) { - int err = efivar_entry_remove(entry); - - if (err) - return err; + efivar_entry_remove(entry); efivar_unregister(entry); return 0; } @@ -615,8 +612,7 @@ static void efivars_sysfs_exit(void) /* Remove all entries and destroy */ int err; - err = __efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list, - NULL, NULL); + err = efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list, NULL); if (err) { pr_err("efivars: Failed to destroy sysfs entries\n"); return; diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index 5640ffa81544..29540013b358 100644 --- a/drivers/firmware/efi/vars.c +++ b/drivers/firmware/efi/vars.c @@ -523,17 +523,10 @@ EXPORT_SYMBOL_GPL(__efivar_entry_add); /** * efivar_entry_remove - remove entry from variable list * @entry: entry to remove from list - * - * Returns 0 on success, or a kernel error code on failure. */ -int efivar_entry_remove(struct efivar_entry *entry) +void efivar_entry_remove(struct efivar_entry *entry) { - if (down_interruptible(&efivars_lock)) - return -EINTR; list_del(&entry->list); - up(&efivars_lock); - - return 0; } EXPORT_SYMBOL_GPL(efivar_entry_remove); diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index 09dfa8362f50..6780fc81cc11 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -180,10 +180,7 @@ fail: static int efivarfs_destroy(struct efivar_entry *entry, void *data) { - int err = efivar_entry_remove(entry); - - if (err) - return err; + efivar_entry_remove(entry); kfree(entry); return 0; } @@ -219,7 +216,7 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc) err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list); if (err) - __efivar_entry_iter(efivarfs_destroy, &efivarfs_list, NULL, NULL); + efivar_entry_iter(efivarfs_destroy, &efivarfs_list, NULL); return err; } @@ -244,7 +241,7 @@ static void efivarfs_kill_sb(struct super_block *sb) kill_litter_super(sb); /* Remove all entries and destroy */ - __efivar_entry_iter(efivarfs_destroy, &efivarfs_list, NULL, NULL); + efivar_entry_iter(efivarfs_destroy, &efivarfs_list, NULL); } static struct file_system_type efivarfs_type = { diff --git a/include/linux/efi.h b/include/linux/efi.h index 08bc6215e3b4..54ca2d6b6c78 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1063,7 +1063,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *), 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); +void efivar_entry_remove(struct efivar_entry *entry); int __efivar_entry_delete(struct efivar_entry *entry); int efivar_entry_delete(struct efivar_entry *entry); -- cgit v1.2.3