| Message ID | 20241210170224.19159-4-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | convert efivarfs to manage object data correctly | expand |
On Tue, Dec 10, 2024 at 9:03 AM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > extern const struct file_operations efivarfs_file_operations; > extern const struct inode_operations efivarfs_dir_inode_operations; > @@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct super_block *sb, > const struct inode *dir, int mode, dev_t dev, > bool is_removable); > > + > + Unnecessary > #endif /* EFIVAR_FS_INTERNAL_H */ > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c > index b22441f7f7c6..dc3870ae784b 100644 > --- a/fs/efivarfs/super.c > +++ b/fs/efivarfs/super.c > @@ -181,6 +181,26 @@ static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name) > return ERR_PTR(-ENOMEM); > } > > +bool efivarfs_variable_is_present(efi_char16_t *variable_name, > + efi_guid_t *vendor, void *data) > +{ > + char *name = efivar_get_utf8name(variable_name, vendor); > + struct super_block *sb = data; > + struct dentry *dentry; > + struct qstr qstr; > + > + if (!name) > + return true; Why is this true? I understand the previous implementation would have hit a null dereference trying to calculate strsize1 on null, so this isn't worse, but if we considered its length to be 0, it would not be found. > + > + qstr.name = name; > + qstr.len = strlen(name); > + dentry = d_hash_and_lookup(sb->s_root, &qstr); > + kfree(name); > + if (dentry) > + dput(dentry); > + return dentry != NULL; > +} > + > static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor, > unsigned long name_size, void *data, > struct list_head *list) > diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c > index 7a07b767e2cc..f6380fdbe173 100644 > --- a/fs/efivarfs/vars.c > +++ b/fs/efivarfs/vars.c > @@ -313,28 +313,6 @@ efivar_variable_is_removable(efi_guid_t vendor, const char *var_name, > return found; > } > > -static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor, > - struct list_head *head) > -{ > - struct efivar_entry *entry, *n; > - unsigned long strsize1, strsize2; > - bool found = false; > - > - strsize1 = ucs2_strsize(variable_name, EFI_VAR_NAME_LEN); > - list_for_each_entry_safe(entry, n, head, list) { > - strsize2 = ucs2_strsize(entry->var.VariableName, EFI_VAR_NAME_LEN); > - if (strsize1 == strsize2 && > - !memcmp(variable_name, &(entry->var.VariableName), > - strsize2) && > - !efi_guidcmp(entry->var.VendorGuid, > - *vendor)) { > - found = true; > - break; > - } > - } > - return found; > -} > - > /* > * Returns the size of variable_name, in bytes, including the > * terminating NULL character, or variable_name_size if no NULL > @@ -439,8 +417,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *, > * we'll ever see a different variable name, > * and may end up looping here forever. > */ > - if (variable_is_present(variable_name, &vendor_guid, > - head)) { > + if (efivarfs_variable_is_present(variable_name, > + &vendor_guid, data)) { > dup_variable_bug(variable_name, &vendor_guid, > variable_name_size); > status = EFI_NOT_FOUND; > -- > 2.35.3 > >
On Tue, 2024-12-10 at 09:14 -0800, Dionna Amalie Glaze wrote: > On Tue, Dec 10, 2024 at 9:03 AM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > extern const struct file_operations efivarfs_file_operations; > > extern const struct inode_operations > > efivarfs_dir_inode_operations; > > @@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct > > super_block *sb, > > const struct inode *dir, int mode, dev_t > > dev, > > bool is_removable); > > > > + > > + > > Unnecessary I can remove the extra line. > > #endif /* EFIVAR_FS_INTERNAL_H */ > > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c > > index b22441f7f7c6..dc3870ae784b 100644 > > --- a/fs/efivarfs/super.c > > +++ b/fs/efivarfs/super.c > > @@ -181,6 +181,26 @@ static struct dentry > > *efivarfs_alloc_dentry(struct dentry *parent, char *name) > > return ERR_PTR(-ENOMEM); > > } > > > > +bool efivarfs_variable_is_present(efi_char16_t *variable_name, > > + efi_guid_t *vendor, void *data) > > +{ > > + char *name = efivar_get_utf8name(variable_name, vendor); > > + struct super_block *sb = data; > > + struct dentry *dentry; > > + struct qstr qstr; > > + > > + if (!name) > > + return true; > > Why is this true? I understand the previous implementation would have > hit a null dereference trying to calculate strsize1 on null, so this > isn't worse, but if we considered its length to be 0, it would not be > found. Because for safety on failure we need to assume a collision. kmalloc failing will already have dropped an error message so adding another here (particularly when the log will likely be filling up with these because we're in a critical memory shortage situation) would seem to be overkill. The memory allocation will never fail ordinarily and if it does the system will be degrading fast, so EFI filesystem variable collision will be the least of the problem. Regards, James
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h index 4b7330b90958..84a36e6fb653 100644 --- a/fs/efivarfs/internal.h +++ b/fs/efivarfs/internal.h @@ -56,6 +56,8 @@ bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data, bool efivar_variable_is_removable(efi_guid_t vendor, const char *name, size_t len); char *efivar_get_utf8name(const efi_char16_t *name16, efi_guid_t *vendor); +bool efivarfs_variable_is_present(efi_char16_t *variable_name, + efi_guid_t *vendor, void *data); extern const struct file_operations efivarfs_file_operations; extern const struct inode_operations efivarfs_dir_inode_operations; @@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct super_block *sb, const struct inode *dir, int mode, dev_t dev, bool is_removable); + + #endif /* EFIVAR_FS_INTERNAL_H */ diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index b22441f7f7c6..dc3870ae784b 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -181,6 +181,26 @@ static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name) return ERR_PTR(-ENOMEM); } +bool efivarfs_variable_is_present(efi_char16_t *variable_name, + efi_guid_t *vendor, void *data) +{ + char *name = efivar_get_utf8name(variable_name, vendor); + struct super_block *sb = data; + struct dentry *dentry; + struct qstr qstr; + + if (!name) + return true; + + qstr.name = name; + qstr.len = strlen(name); + dentry = d_hash_and_lookup(sb->s_root, &qstr); + kfree(name); + if (dentry) + dput(dentry); + return dentry != NULL; +} + static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor, unsigned long name_size, void *data, struct list_head *list) diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c index 7a07b767e2cc..f6380fdbe173 100644 --- a/fs/efivarfs/vars.c +++ b/fs/efivarfs/vars.c @@ -313,28 +313,6 @@ efivar_variable_is_removable(efi_guid_t vendor, const char *var_name, return found; } -static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor, - struct list_head *head) -{ - struct efivar_entry *entry, *n; - unsigned long strsize1, strsize2; - bool found = false; - - strsize1 = ucs2_strsize(variable_name, EFI_VAR_NAME_LEN); - list_for_each_entry_safe(entry, n, head, list) { - strsize2 = ucs2_strsize(entry->var.VariableName, EFI_VAR_NAME_LEN); - if (strsize1 == strsize2 && - !memcmp(variable_name, &(entry->var.VariableName), - strsize2) && - !efi_guidcmp(entry->var.VendorGuid, - *vendor)) { - found = true; - break; - } - } - return found; -} - /* * Returns the size of variable_name, in bytes, including the * terminating NULL character, or variable_name_size if no NULL @@ -439,8 +417,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *, * we'll ever see a different variable name, * and may end up looping here forever. */ - if (variable_is_present(variable_name, &vendor_guid, - head)) { + if (efivarfs_variable_is_present(variable_name, + &vendor_guid, data)) { dup_variable_bug(variable_name, &vendor_guid, variable_name_size); status = EFI_NOT_FOUND;
Instead of searching the variable entry list for a variable, use the dcache lookup functions to find it instead. Also add an efivarfs_ prefix to the function now it is no longer static. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- fs/efivarfs/internal.h | 4 ++++ fs/efivarfs/super.c | 20 ++++++++++++++++++++ fs/efivarfs/vars.c | 26 ++------------------------ 3 files changed, 26 insertions(+), 24 deletions(-)