We can cause an oops by unlinking an open efivarfs file, then reading
(or writing) from the deleted file. The unlink operation causes the
efivarfs_entry struct to be freed, which is referenced again in the
read.
This change allow the efivar_entry structures to remain present after
the variable has been removed, but flagged as not present in firmware by
way of having an empty ->list member. We keep a refcount to determine
when the entry can actually be freed.
Signed-off-by: Jeremy Kerr <jk@xxxxxxxxxx>
---
drivers/firmware/efivars.c | 105 +++++++++++++++++++++++++++++++------
1 file changed, 89 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 053f28b..764f548 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -121,6 +121,15 @@ struct efi_variable {
struct efivar_entry {
struct efivars *efivars;
struct efi_variable var;
+
+ /* references to the kref are held by:
+ * - open efivarfs files
+ * - efivarfs inodes
+ * - the kobj
+ * - presence in the global efivar list
+ */
+ struct kref kref;
+
struct list_head list;
struct kobject kobj;
};
@@ -147,7 +156,8 @@ struct efivar_attribute efivar_attr_##_name = { \
};
#define to_efivar_attr(_attr) container_of(_attr, struct efivar_attribute, attr)
-#define to_efivar_entry(obj) container_of(obj, struct efivar_entry, kobj)
+#define kobj_to_efivar_entry(obj) container_of(obj, struct efivar_entry, kobj)
+#define kref_to_efivar_entry(obj) container_of(obj, struct efivar_entry, kref)
/*
* Prototype for sysfs creation function
@@ -580,7 +590,7 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
static ssize_t efivar_attr_show(struct kobject *kobj, struct attribute *attr,
char *buf)
{
- struct efivar_entry *var = to_efivar_entry(kobj);
+ struct efivar_entry *var = kobj_to_efivar_entry(kobj);
struct efivar_attribute *efivar_attr = to_efivar_attr(attr);
ssize_t ret = -EIO;
@@ -596,7 +606,7 @@ static ssize_t efivar_attr_show(struct kobject *kobj, struct attribute *attr,
static ssize_t efivar_attr_store(struct kobject *kobj, struct attribute *attr,
const char *buf, size_t count)
{
- struct efivar_entry *var = to_efivar_entry(kobj);
+ struct efivar_entry *var = kobj_to_efivar_entry(kobj);
struct efivar_attribute *efivar_attr = to_efivar_attr(attr);
ssize_t ret = -EIO;
@@ -614,12 +624,28 @@ static const struct sysfs_ops efivar_attr_ops = {
.store = efivar_attr_store,
};
-static void efivar_release(struct kobject *kobj)
+static void efivar_entry_release(struct kref *kref)
{
- struct efivar_entry *var = container_of(kobj, struct efivar_entry, kobj);
+ struct efivar_entry *var = kref_to_efivar_entry(kref);
kfree(var);
}
+static void efivar_entry_put(struct efivar_entry *entry)
+{
+ kref_put(&entry->kref, efivar_entry_release);
+}
+
+static void efivar_entry_get(struct efivar_entry *entry)
+{
+ kref_get(&entry->kref);
+}
+
+static void efivar_release(struct kobject *kobj)
+{
+ struct efivar_entry *var = kobj_to_efivar_entry(kobj);
+ efivar_entry_put(var);
+}
+
static EFIVAR_ATTR(guid, 0400, efivar_guid_read, NULL);
static EFIVAR_ATTR(attributes, 0400, efivar_attr_read, NULL);
static EFIVAR_ATTR(size, 0400, efivar_size_read, NULL);
@@ -647,9 +673,22 @@ efivar_unregister(struct efivar_entry *var)
kobject_put(&var->kobj);
}
+static bool efivar_is_present(struct efivar_entry *entry)
+{
+ bool present;
+
+ spin_lock(&entry->efivars->lock);
+ present = !list_empty(&entry->list);
+ spin_unlock(&entry->efivars->lock);
+
+ return present;
+}
+
static int efivarfs_file_open(struct inode *inode, struct file *file)
{
- file->private_data = inode->i_private;
+ struct efivar_entry *entry = inode->i_private;
+ file->private_data = entry;
+ efivar_entry_get(entry);
return 0;
}
@@ -706,6 +745,15 @@ static ssize_t efivarfs_file_write(struct file *file,
if (attributes & ~(EFI_VARIABLE_MASK))
return -EINVAL;
+ /*
+ * It's possible that the variable has been removed from firmware
+ * since the open(), in which case we need to flag an error
+ * TODO: allow writes that would reinstate the variable, but check
+ * against duplicate variables...
+ */
+ if (!efivar_is_present(var))
+ return -EIO;
+
efivars = var->efivars;
/*
@@ -789,9 +837,10 @@ static ssize_t efivarfs_file_write(struct file *file,
mutex_unlock(&inode->i_mutex);
} else if (status == EFI_NOT_FOUND) {
- list_del(&var->list);
+ list_del_init(&var->list);
spin_unlock(&efivars->lock);
efivar_unregister(var);
+ efivar_entry_put(var);
drop_nlink(inode);
d_delete(file->f_dentry);
dput(file->f_dentry);
@@ -819,6 +868,13 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
void *data;
ssize_t size = 0;
+ /*
+ * It's possible that the variable has been removed from firmware
+ * since the open(), in which case we need to flag an error.
+ */
+ if (!efivar_is_present(var))
+ return -EIO;
+
spin_lock(&efivars->lock);
status = efivars->ops->get_variable(var->var.VariableName,
&var->var.VendorGuid,
@@ -854,8 +910,15 @@ out_free:
return size;
}
+static int efivarfs_file_release(struct inode *inode, struct file *file)
+{
+ efivar_entry_put(file->private_data);
+ return 0;
+}
+
static void efivarfs_evict_inode(struct inode *inode)
{
+ efivar_entry_put(inode->i_private);
clear_inode(inode);
}
@@ -871,10 +934,11 @@ static struct super_block *efivarfs_sb;
static const struct inode_operations efivarfs_dir_inode_operations;
static const struct file_operations efivarfs_file_operations = {
- .open = efivarfs_file_open,
- .read = efivarfs_file_read,
- .write = efivarfs_file_write,
- .llseek = no_llseek,
+ .open = efivarfs_file_open,
+ .read = efivarfs_file_read,
+ .write = efivarfs_file_write,
+ .release = efivarfs_file_release,
+ .llseek = no_llseek,
};
static struct inode *efivarfs_get_inode(struct super_block *sb,
@@ -946,6 +1010,8 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
goto out;
}
+ kref_init(&var->kref);
+
/* length of the variable name itself: remove GUID and separator */
namelen = dentry->d_name.len - GUID_LEN - 1;
@@ -969,6 +1035,7 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
kobject_uevent(&var->kobj, KOBJ_ADD);
spin_lock(&efivars->lock);
list_add(&var->list, &efivars->list);
+ efivar_entry_get(var);
spin_unlock(&efivars->lock);
d_instantiate(dentry, inode);
dget(dentry);
@@ -993,9 +1060,10 @@ static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
0, 0, NULL);
if (status == EFI_SUCCESS || status == EFI_NOT_FOUND) {
- list_del(&var->list);
+ list_del_init(&var->list);
spin_unlock(&efivars->lock);
efivar_unregister(var);
+ efivar_entry_put(var);
drop_nlink(dentry->d_inode);
dput(dentry);
return 0;
@@ -1077,6 +1145,7 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
mutex_lock(&inode->i_mutex);
inode->i_private = entry;
+ efivar_entry_get(entry);
i_size_write(inode, size+4);
mutex_unlock(&inode->i_mutex);
d_add(dentry, inode);
@@ -1221,8 +1290,10 @@ static int efi_pstore_write(enum pstore_type_id type,
0, NULL);
}
- if (found)
- list_del(&found->list);
+ if (found) {
+ list_del_init(&found->list);
+ efivar_entry_put(found);
+ }
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];
@@ -1414,7 +1485,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
spin_unlock(&efivars->lock);
return -EIO;
}
- list_del(&search_efivar->list);
+ list_del_init(&search_efivar->list);
/* We need to release this lock before unregistering. */
spin_unlock(&efivars->lock);
efivar_unregister(search_efivar);
@@ -1533,6 +1604,7 @@ efivar_create_sysfs_entry(struct efivars *efivars,
spin_lock(&efivars->lock);
list_add(&new_efivar->list, &efivars->list);
+ efivar_entry_get(new_efivar);
spin_unlock(&efivars->lock);
return 0;
@@ -1603,8 +1675,9 @@ void unregister_efivars(struct efivars *efivars)
list_for_each_entry_safe(entry, n, &efivars->list, list) {
spin_lock(&efivars->lock);
- list_del(&entry->list);
+ list_del_init(&entry->list);
spin_unlock(&efivars->lock);
+ efivar_entry_put(entry);
efivar_unregister(entry);
}
if (efivars->new_var)
--
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
